Stream: git-wasmtime

Topic: wasmtime / PR #8990 Add v128.const support to Winch

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 18:25):

jeffcharles opened PR #8990 from jeffcharles:winch-add-v128-const to bytecodealliance:main:

<!--
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
-->
Adding support for v128.const to Winch. Start of work to add SIMD support to Winch for the x64 architecture.

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

jeffcharles submitted PR review.

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

jeffcharles created PR review comment:

Not sure if we want to keep the wasm_features(simd) check on here since this is a SIMD test. I removed it so the test would run on Winch.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 18:27):

jeffcharles submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 18:27):

jeffcharles created PR review comment:

This is a result of changing how memory values are popped from the stack. It now uses the type of the Wasm value to determine the scratch register to use so this now uses the XMM scratch register instead of a GPR.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 18:27):

jeffcharles submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 18:27):

jeffcharles created PR review comment:

Ditto for this change to the disass.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 18:29):

jeffcharles submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 18:29):

jeffcharles created PR review comment:

This now needs to handle 128 bit values so using the GPR scratch won't work.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 18:29):

saulecabrera requested saulecabrera for a review on PR #8990.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 18:56):

jeffcharles has marked PR #8990 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 18:56):

jeffcharles requested elliottt for a review on PR #8990.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 18:56):

jeffcharles requested wasmtime-compiler-reviewers for a review on PR #8990.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 18:56):

jeffcharles requested wasmtime-core-reviewers for a review on PR #8990.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2024 at 21:44):

github-actions[bot] commented on PR #8990:

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "winch"

Thus the following users have been cc'd because of the following labels:

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 14:10):

saulecabrera submitted PR review:

It's going on a good direction!

One thing that we need to also update as part of this PR is the stack alignment details in the ABI.

Currently, the stack alignment and slot size are both set to 8 bytes, given that types > 8 bytes were not supported. We'd probably want to ensure that it's now set to 16.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 14:10):

saulecabrera submitted PR review:

It's going on a good direction!

One thing that we need to also update as part of this PR is the stack alignment details in the ABI.

Currently, the stack alignment and slot size are both set to 8 bytes, given that types > 8 bytes were not supported. We'd probably want to ensure that it's now set to 16.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 14:10):

saulecabrera created PR review comment:

I've been giving a bit of thought to this change. If I'm not wrong the motivation is to future-proof, be explicit and correctly differentiate the scratch register for different types (GPR, FPR) and sizes (FPR vs v128).

I'm wondering if there's an alternative way that would allow a similar outcome without need to introduce more *_scratch_reg variations. One option that I can think of is to remove scratch_reg and float_scratch_reg and rely exclusively on scratch_for and sratch!(M, &ty) method and macro respectively. This might make it easier to encapsulate the details of which register should be used depending on the type/size/architecture.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 14:14):

saulecabrera edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 14:20):

jeffcharles submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 14:20):

jeffcharles created PR review comment:

Yeah I debated whether to just use the float scratch register directly but that seemed like it would be mixing different levels of abstraction. Changing everything to use scratch_for or scratch!(M, &ty) instead makes sense to me. I'll make those changes. Might try doing that in a separate PR to keep it easier to review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 15:31):

jeffcharles submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 15:31):

jeffcharles created PR review comment:

I looked into this a little more, the scratch! macro relies on scratch_for and scratch_for relies on the scratch_reg and float_scratch_reg methods being present on the ABI trait. I could remove scratch_reg and float_scratch_reg from the trait and move the implementation of scratch_for into the ABI trait implementations. The logic isn't that complicated so repeating the logic in each trait implementation with the registers hard-coded wouldn't necessarily be too bad.

Alternatively, I could not add vector_scratch_reg on the trait, update scratch_for to use the float_scratch_register for v128, and update store_impl to use scratch_for. That'll avoid having to implement scratch_for for each ABI trait implementation.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 15:38):

saulecabrera submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 15:38):

saulecabrera created PR review comment:

In my original comment I was leaning more towards the first option from your comment above: moving scratch_for to each ABI implementation, hoping to (i) get the most flexibility (architecture-wise) at the cost of some repetition (ii) reduce the number of scratch related methods from the ABI trait.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 16:10):

jeffcharles submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 16:10):

jeffcharles created PR review comment:

Opened https://github.com/bytecodealliance/wasmtime/pull/8999 to do the refactoring.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 17:50):

jeffcharles updated PR #8990.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 17:53):

jeffcharles submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 17:53):

jeffcharles created PR review comment:

I could change this to just reuse the existing floating point register methods given this is inside the x64 ABI if that would be preferable.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 17:55):

jeffcharles commented on PR #8990:

The merge commit removed the new vector method on the ABI trait. Not sure if you would like me to do the same with the next_vr and associated methods I've added in the X64ABI file.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 17:55):

jeffcharles requested saulecabrera for a review on PR #8990.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 18:02):

saulecabrera commented on PR #8990:

The merge commit removed the new vector method on the ABI trait. Not sure if you would like me to do the same with the next_vr and associated methods I've added in the X64ABI file.

Yeah it sounds reasonable to me to get rid of next_vr and vector_reg_for 👍🏽

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 18:11):

jeffcharles updated PR #8990.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 18:23):

jeffcharles updated PR #8990.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 23 2024 at 18:32):

jeffcharles updated PR #8990.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 24 2024 at 17:48):

jeffcharles updated PR #8990.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 24 2024 at 18:28):

jeffcharles updated PR #8990.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 24 2024 at 18:52):

saulecabrera submitted PR review:

This looks great to me, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Jul 24 2024 at 19:08):

saulecabrera merged PR #8990.

Last updated: Oct 23 2024 at 20:03 UTC