Stream: git-wasmtime

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

Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

saulecabrera requested cfallin for a review on PR #6860.

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:

• Constant pool: A constant pool is implemented, at the Assembler level, using the machinery exposed by Cranelift's VCode and MachBuffer. Float immediates are stored using their bit representation in the value stack, and whenever they are used at the MacroAssembler level they are added to the constant pool, from that point on, they are referenced through a Constant addressing mode, which gets translated to a RIP-relative addressing mode during emission.

• More precise value tagging: aside from immediates, from which the type can be easily inferred, all the other value stack entries (Memory, Reg, and Local) are modified to explicitly contain a WebAssembly type. This allows for better instruction selection.

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

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

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
-->

Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

saulecabrera requested elliottt for a review on PR #6860.

Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

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

Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

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

Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

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

Wasmtime GitHub notifications bot (Aug 18 2023 at 19:22):

saulecabrera requested alexcrichton for a review on PR #6860.

Wasmtime GitHub notifications bot (Aug 18 2023 at 19:50):

saulecabrera updated PR #6860.

Wasmtime GitHub notifications bot (Aug 18 2023 at 21:18):

saulecabrera updated PR #6860.

Wasmtime GitHub notifications bot (Aug 18 2023 at 21:42):

cfallin submitted PR review:

Generally very clean -- thanks! Some thoughts below.

Wasmtime GitHub notifications bot (Aug 18 2023 at 21:42):

cfallin submitted PR review:

Generally very clean -- thanks! Some thoughts below.

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...

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?

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)?

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?

Wasmtime GitHub notifications bot (Aug 22 2023 at 01:08):

saulecabrera updated PR #6860.

Wasmtime GitHub notifications bot (Aug 22 2023 at 01:10):

saulecabrera updated PR #6860.

Wasmtime GitHub notifications bot (Aug 22 2023 at 18:26):

saulecabrera updated PR #6860.

Wasmtime GitHub notifications bot (Aug 22 2023 at 19:29):

saulecabrera updated PR #6860.

Wasmtime GitHub notifications bot (Aug 22 2023 at 20:10):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Aug 22 2023 at 20:10):

saulecabrera created PR review comment:

Good call, fixed!

Wasmtime GitHub notifications bot (Aug 22 2023 at 20:12):

saulecabrera submitted PR review.

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

Wasmtime GitHub notifications bot (Aug 22 2023 at 20:13):

saulecabrera submitted PR review.

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.

Wasmtime GitHub notifications bot (Aug 22 2023 at 20:14):

saulecabrera submitted PR review.

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.

Wasmtime GitHub notifications bot (Aug 22 2023 at 20:14):

saulecabrera requested cfallin for a review on PR #6860.

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.

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.

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?

Wasmtime GitHub notifications bot (Aug 23 2023 at 09:56):

saulecabrera updated PR #6860.

Wasmtime GitHub notifications bot (Aug 23 2023 at 09:56):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Aug 23 2023 at 09:56):

saulecabrera created PR review comment:

Fixed!

Wasmtime GitHub notifications bot (Aug 23 2023 at 09:57):

saulecabrera has enabled auto merge for PR #6860.

Wasmtime GitHub notifications bot (Aug 23 2023 at 11:24):

saulecabrera merged PR #6860.

Last updated: Nov 22 2024 at 16:03 UTC