Stream: git-wasmtime

Topic: wasmtime / PR #6860 winch: Initial support for floats


view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

saulecabrera requested cfallin for a review on PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

saulecabrera opened PR #6860 from saulecabrera:winch-float-consts to bytecodealliance:main:

This change introduces the necessary building blocks to support floats in Winch as well as support for both f32.const and f64.const instructions.

To achieve support for floats, this change adds several key enhancements to the compiler:

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

saulecabrera requested elliottt for a review on PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

saulecabrera requested wasmtime-fuzz-reviewers for a review on PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

saulecabrera requested wasmtime-compiler-reviewers for a review on PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

saulecabrera requested wasmtime-core-reviewers for a review on PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

saulecabrera requested alexcrichton for a review on PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 19:50):

saulecabrera updated PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 21:18):

saulecabrera updated PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 21:42):

cfallin submitted PR review:

Generally very clean -- thanks! Some thoughts below.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 21:42):

cfallin submitted PR review:

Generally very clean -- thanks! Some thoughts below.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 21:42):

cfallin created PR review comment:

rather than duplicate code-paths for gpr and fpr variants, would it make sense to have an array indexed by class? Either regalloc2's RegClass could be used or a custom type here, doesn't matter too much...

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 21:42):

cfallin created PR review comment:

There's some subtlety here in that only the lower half of the v8..v15 registers are callee-saved. In Cranelift we overapproximate on both sides (when saving, save the whole register; when generating a callsite, assume whole register is clobbered), and then have an optimization where if our own function's ABI and the callee's ABI are the same then at callsites we ignore these for clobbers. I haven't thought through all the implications with Winch's regalloc model but I just wanted to make sure we've considered this here?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 21:42):

cfallin created PR review comment:

I'm not sure exactly (I haven't paged in enough details of the constant pool) but thought I should call this XXX comment out -- can we resolve it either way (do the refactor or remove the comment and note why we're doing this)?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 18 2023 at 21:42):

cfallin created PR review comment:

now that this exists, can we rewrite register_constants to use it?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 01:08):

saulecabrera updated PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 01:10):

saulecabrera updated PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 18:26):

saulecabrera updated PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 19:29):

saulecabrera updated PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 20:10):

saulecabrera submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 20:10):

saulecabrera created PR review comment:

Good call, fixed!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 20:12):

saulecabrera submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 20:12):

saulecabrera created PR review comment:

I've added class based indexing to RegSet, it indeed removed quite some duplication. I've opted to keep CodeGenContext::any_gpr and rewrote it in terms of CodeGenContext::reg_for_class

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 20:13):

saulecabrera submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 20:13):

saulecabrera created PR review comment:

Yeah, this comment is old and not applicable. I've removed it.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 20:14):

saulecabrera submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 20:14):

saulecabrera created PR review comment:

Agreed, as discussed offline, I've added size information to the callee-saved registers.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 20:14):

saulecabrera requested cfallin for a review on PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 20:26):

cfallin submitted PR review:

Looks great overall, thanks!

Just one question left for me re: stack alignment, below; otherwise good to merge.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 20:26):

cfallin submitted PR review:

Looks great overall, thanks!

Just one question left for me re: stack alignment, below; otherwise good to merge.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 22 2023 at 20:26):

cfallin created PR review comment:

I'm wondering now, with type-based spills, if this has the potential to misalign the stack given that F32s take 4 bytes. Could you add a comment here about alignment noting how this works?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 23 2023 at 09:56):

saulecabrera updated PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 23 2023 at 09:56):

saulecabrera submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 23 2023 at 09:56):

saulecabrera created PR review comment:

Fixed!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 23 2023 at 09:57):

saulecabrera has enabled auto merge for PR #6860.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 23 2023 at 11:24):

saulecabrera merged PR #6860.


Last updated: Dec 23 2024 at 12:05 UTC