Stream: git-wasmtime

Topic: wasmtime / PR #10834 x64: Migrate xmm mov-family instruct...

Wasmtime GitHub notifications bot (May 25 2025 at 21:33):

alexcrichton requested abrown for a review on PR #10834.

Wasmtime GitHub notifications bot (May 25 2025 at 21:33):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #10834.

Wasmtime GitHub notifications bot (May 25 2025 at 21:33):

alexcrichton opened PR #10834 from alexcrichton:x64-xmm-mov to bytecodealliance:main:

This commit migrates xmm movement-style instructions, such as mov{s{s,d},{a,u}p{s,d},dq{a,u}} to the new assembler. This enables deleting the XmmMovRM class of instructions in ISLE. Along the way a number of notable changes were made:

• The emission of XmmCmove now properly uses movss for 32-bit floats instead of movsd (accident from #4317)
• ISLE constructors for assembler instructions now take SyntheticAmode instead of Amode since it's already supported anyway and it's a more flexible argument to take.
• Some Into impls were swapped to From impls since that's a more generic way of implementing the Into and From traits.
• The conversion from SyntheticAmode to the assembler Amode<Gpr> was fixed where one variant needed to use rsp instead of rbp.
• The is_move method was updated to ignore movss and movsd instructions since they're not true movement-related instructions in register-to-register situations.

<!--
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 (May 25 2025 at 21:33):

alexcrichton requested wasmtime-core-reviewers for a review on PR #10834.

Wasmtime GitHub notifications bot (May 25 2025 at 21:33):

alexcrichton requested dicej for a review on PR #10834.

Wasmtime GitHub notifications bot (May 25 2025 at 21:33):

alexcrichton commented on PR #10834:

@abrown IIRC you said you were working on mov instructions, which I'm realize now I assumed meant integer-related ones, but if this overlaps with what you've done I'm happy to close this and defer to your work

Wasmtime GitHub notifications bot (May 25 2025 at 22:45):

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

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "cranelift:meta", "isle"

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

• cfallin: isle
• fitzgen: isle

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

Learn more.
</details>

Wasmtime GitHub notifications bot (May 26 2025 at 02:16):

alexcrichton updated PR #10834.

Wasmtime GitHub notifications bot (May 27 2025 at 15:41):

alexcrichton edited PR #10834:

This commit migrates xmm movement-style instructions, such as mov{s{s,d},{a,u}p{s,d},dq{a,u}} to the new assembler. This enables deleting the XmmMovRM class of instructions in ISLE. Along the way a number of notable changes were made:

• The emission of XmmCmove now properly uses movss for 32-bit floats instead of movsd (accident from #4317)
• ISLE constructors for assembler instructions now take SyntheticAmode instead of Amode since it's already supported anyway and it's a more flexible argument to take.
• Some Into impls were swapped to From impls since that's a more generic way of implementing the Into and From traits. moved to https://github.com/bytecodealliance/wasmtime/pull/10837
• The conversion from SyntheticAmode to the assembler Amode<Gpr> was fixed where one variant needed to use rsp instead of rbp.
• The is_move method was updated to ignore movss and movsd instructions since they're not true movement-related instructions in register-to-register situations.

<!--
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 (May 27 2025 at 15:48):

alexcrichton edited PR #10834:

This commit migrates xmm movement-style instructions, such as mov{s{s,d},{a,u}p{s,d},dq{a,u}} to the new assembler. This enables deleting the XmmMovRM class of instructions in ISLE. Along the way a number of notable changes were made:

• The emission of XmmCmove now properly uses movss for 32-bit floats instead of movsd (accident from #4317) split out into https://github.com/bytecodealliance/wasmtime/pull/10839
• ISLE constructors for assembler instructions now take SyntheticAmode instead of Amode since it's already supported anyway and it's a more flexible argument to take.
• Some Into impls were swapped to From impls since that's a more generic way of implementing the Into and From traits. moved to https://github.com/bytecodealliance/wasmtime/pull/10837
• The conversion from SyntheticAmode to the assembler Amode<Gpr> was fixed where one variant needed to use rsp instead of rbp.
• The is_move method was updated to ignore movss and movsd instructions since they're not true movement-related instructions in register-to-register situations.

<!--
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 (May 27 2025 at 15:48):

alexcrichton commented on PR #10834:

Converting this to a draft, thinking it's best to split pieces out into separate PRs.

Wasmtime GitHub notifications bot (May 27 2025 at 16:51):

alexcrichton updated PR #10834.

Wasmtime GitHub notifications bot (May 27 2025 at 16:51):

alexcrichton edited PR #10834:

This commit migrates xmm movement-style instructions, such as mov{s{s,d},{a,u}p{s,d},dq{a,u}} to the new assembler. This enables deleting the XmmMovRM class of instructions in ISLE. Along the way a number of notable changes were made:

• ISLE constructors for assembler instructions now take SyntheticAmode instead of Amode since it's already supported anyway and it's a more flexible argument to take.
• The conversion from SyntheticAmode to the assembler Amode<Gpr> was fixed where one variant needed to use rsp instead of rbp.
• The is_move method was updated to ignore movss and movsd instructions since they're not true movement-related instructions in register-to-register situations.

<!--
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 (May 27 2025 at 16:51):

alexcrichton has marked PR #10834 as ready for review.

Wasmtime GitHub notifications bot (May 27 2025 at 16:52):

alexcrichton commented on PR #10834:

Ok just kidding, back to being ready. I ideally wanted to split out the SyntheticAmode and update from rbp to rsp, but there was nothing exercising it previously so I think it makes sense to include here instead.

Wasmtime GitHub notifications bot (May 28 2025 at 01:37):

abrown assigned abrown to PR #10834.

Wasmtime GitHub notifications bot (May 28 2025 at 01:40):

abrown commented on PR #10834:

@abrown IIRC you said you were working on mov instructions, which I'm realize now I assumed meant integer-related ones, but if this overlaps with what you've done I'm happy to close this and defer to your work

I did have some of these as well but I'm happy to rebase on this because hadn't made it to the "fix up all the Rust uses" part. Speaking of: as I glanced at this, does it make sense to use Inst::gen_move more liberally? Or perhaps add Inst::load and Inst::store? Some of the Rust bits seemed like code that we could hide away: "unwrap four levels, rewrap with new types." I'll take a closer look at this tomorrow!

Wasmtime GitHub notifications bot (May 28 2025 at 16:12):

alexcrichton updated PR #10834 (assigned to abrown).

Wasmtime GitHub notifications bot (May 28 2025 at 16:15):

alexcrichton commented on PR #10834:

Good point! I updated Winch to use gen_move, but load/store are currently pub(crate) and inaccessible to Winch. I'm happy to make those pub but also we just talked in the Cranelift meeting about having less pub so I hesitated and figured I'd avoid changing it for now

Wasmtime GitHub notifications bot (May 28 2025 at 17:27):

abrown submitted PR review:

Looks good to me! I'll add the scalar moves on top of this.

Wasmtime GitHub notifications bot (May 28 2025 at 17:47):

abrown merged PR #10834.

Last updated: Dec 06 2025 at 06:05 UTC