Stream: git-wasmtime

Topic: wasmtime / PR #10856 Add round variants to the new assembler

Wasmtime GitHub notifications bot (May 29 2025 at 00:42):

jlb6740 opened PR #10856 from jlb6740:add-rounding-to-new-assembler to bytecodealliance:main:

<!--
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 29 2025 at 00:42):

jlb6740 requested cfallin for a review on PR #10856.

Wasmtime GitHub notifications bot (May 29 2025 at 00:42):

jlb6740 requested wasmtime-compiler-reviewers for a review on PR #10856.

Wasmtime GitHub notifications bot (May 29 2025 at 00:42):

jlb6740 commented on PR #10856:

Will add vround variants in a follow-up to this patch.

Wasmtime GitHub notifications bot (May 29 2025 at 00:43):

jlb6740 commented on PR #10856:

Only touches the assembler part.

Wasmtime GitHub notifications bot (May 29 2025 at 00:43):

jlb6740 deleted a comment on PR #10856:

Only touches the assembler part.

Wasmtime GitHub notifications bot (May 29 2025 at 00:43):

jlb6740 edited a comment on PR #10856:

Will add vround variants in a follow-up to this patch. Also, this only touches the assembler and does not connect isle.

Wasmtime GitHub notifications bot (May 30 2025 at 00:02):

jlb6740 updated PR #10856.

Wasmtime GitHub notifications bot (May 30 2025 at 00:04):

jlb6740 edited a comment on PR #10856:

This only touches the assembler and does not connect isle.

Wasmtime GitHub notifications bot (May 30 2025 at 20:08):

jlb6740 updated PR #10856.

Wasmtime GitHub notifications bot (May 30 2025 at 20:14):

jlb6740 updated PR #10856.

Wasmtime GitHub notifications bot (May 30 2025 at 20:23):

jlb6740 updated PR #10856.

Wasmtime GitHub notifications bot (May 30 2025 at 20:35):

jlb6740 updated PR #10856.

Wasmtime GitHub notifications bot (May 30 2025 at 21:19):

abrown submitted PR review.

Wasmtime GitHub notifications bot (May 30 2025 at 21:21):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 30 2025 at 21:21):

alexcrichton created PR review comment:

Should these be align(xmm_m128)?

(should we maybe have a check in the DSL that non-avx xmm_m128 arguments are required to be align?)

Wasmtime GitHub notifications bot (May 30 2025 at 21:34):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 30 2025 at 21:34):

jlb6740 created PR review comment:

I read this comment multiple times trying to interpret:

"NB: the non-AVX variant of this instruction does not require that the operand
;; is aligned, but the instruction variant used here requires it to be aligned."

Perhaps I should have asked. So the non-AVX variant here does require alignment?

Wasmtime GitHub notifications bot (May 30 2025 at 21:35):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (May 30 2025 at 21:38):

alexcrichton created PR review comment:

Ah yeah I believe that comment is referring to rounds{s,d}, as opposed to roundp{s,d}. I'm under the impression that the f32/f64 versions do not require aligned memory but the 128-bit versions require aligned memory. The comment in ISLE I believe can be deleted with the new instructions since it's correctly modeled.

This reminds me as well, should rounds{s,d} below use xmm_m{32,64} instead of xmm_m128? We don't have anything actually checking that right now but with PCC in the future (or some other integration) it might matter to annotate that with the appropriate bit-width.

Wasmtime GitHub notifications bot (May 30 2025 at 21:38):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 30 2025 at 21:42):

jlb6740 updated PR #10856.

Wasmtime GitHub notifications bot (May 30 2025 at 21:43):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (May 30 2025 at 21:43):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (May 30 2025 at 21:52):

abrown merged PR #10856.

Wasmtime GitHub notifications bot (May 30 2025 at 22:05):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (May 30 2025 at 22:05):

jlb6740 created PR review comment:

@alexcrichton Yes you are right on the rounds{s,d}. I am not sure how this would come in to play but let me update this to the correct annotation and also maybe remove that comment.

Wasmtime GitHub notifications bot (May 30 2025 at 22:05):

jlb6740 edited PR review comment.

Wasmtime GitHub notifications bot (May 30 2025 at 22:09):

alexcrichton created PR review comment:

Oh sorry fully reading your comment I can also clarify more:

The documentation there referred to the ISLE-specific abstraction of XmmUnaryRmRImm which internally stores XmmMemAligned. The roundss and roundsd instructions used XmmUnaryRmRImm but this was technically not a correct way to model these instructions. The roundss and roundsd instructions do not actually require alignment. The problem, and where the comment comes from, is that the way this happens in ISLE is that first a Value is converted to XmmMem as the load is "sinkable". Next it goes to the x64_round helper which takes XmmMem. Next, without AVX, it would go to XmmUnaryRmRImm, but this required converting the XmmMem to XmmMemAligned. That, for all WebAssembly loads/stores, would automatically do an unaligned load and then use a register operand. The problem with this is that the automatic load inserted would load 128-bits, not the appropriate number of bits for the f32/f64.

So basically that long comment was an artifact of a precisely how ISLE is set up today. None of it is applicable under the new assembler, and I think that the roundps and roundpd instructions need align(xmm_m128) but roundss and roundsd can use xmm_m32 and xmm_m64 since they don't require alignment.

Also FWIW I see a new commit here but I don't believe it landed because it was pushed after this was already in the merge queue, so mind filing a follow-up commit with that?

Wasmtime GitHub notifications bot (May 30 2025 at 22:10):

alexcrichton submitted PR review.

Last updated: Dec 06 2025 at 07:03 UTC