Stream: git-wasmtime

Topic: wasmtime / PR #10829 winch: Crank the ratchet on tests th...

Wasmtime GitHub notifications bot (May 23 2025 at 00:25):

alexcrichton opened PR #10829 from alexcrichton:winch-ratchet to bytecodealliance:main:

This commit moves all "ignore these outright" tests for Winch and AArch64 to "run the test and expect failure" after the fix in #10826 seems to have fixed many of them. Two failures remained which were easy enough to fix here:

• The cmov implementation only worked on integer registers, not floating-point registers, leading to a panic. Some small instruction helpers were added for floating-point selects to resolve this panic and get some tests passing.

• More subtly the calculation of a table's function address was calculating the wrong value on AArch64. This happened because the mul low-level helper on AArch64 required use of the scratch register when an immediate was an operand, unlike on x86_64. The table address helper was also using the scratch register for other data, however, which meant that loading an immediate into scratch clobbered the previous data. The fix here was to allocate a temporary register in the function address routine.

After these fixes the ignore method is no longer necessary as all tests can be run and be flagged as either pass or fail. This additionally adds assertions in the asm.rs layer that any usage of regs::scratch() has an assertion that the other operand isn't also the scratch register to help prevent this issue from cropping back up. This identified one other location in fuel handling where the scratch register collided.

<!--
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 23 2025 at 00:25):

alexcrichton requested abrown for a review on PR #10829.

Wasmtime GitHub notifications bot (May 23 2025 at 00:25):

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

Wasmtime GitHub notifications bot (May 23 2025 at 00:25):

alexcrichton requested dicej for a review on PR #10829.

Wasmtime GitHub notifications bot (May 23 2025 at 00:25):

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

Wasmtime GitHub notifications bot (May 23 2025 at 00:25):

alexcrichton requested saulecabrera for a review on PR #10829.

Wasmtime GitHub notifications bot (May 23 2025 at 00:36):

alexcrichton updated PR #10829.

Wasmtime GitHub notifications bot (May 23 2025 at 02:19):

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

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:

• saulecabrera: winch

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 23 2025 at 13:56):

saulecabrera submitted PR review:

Glad that we can avoid ignoring tests. Thanks for this! Left one small comment inline.

Wasmtime GitHub notifications bot (May 23 2025 at 13:56):

saulecabrera created PR review comment:

One thing to note here is that this will potentially increase register pressure for x64, but in reality we're trying to fix an Aarch64 issue with immediate value handling. I don't think this is a blocker, but wanted to point this out since this will likely change in the near-future since I have a work-in-progress branch which fixes the scratch register handling more broadly and will hopefully allow us to keep the previous implementation for x64.

Wasmtime GitHub notifications bot (May 23 2025 at 13:58):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (May 23 2025 at 13:58):

alexcrichton created PR review comment:

Oh nice! Would you prefer to hold off on landing this until said refactoring has landed?

Wasmtime GitHub notifications bot (May 23 2025 at 15:00):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (May 23 2025 at 15:00):

saulecabrera created PR review comment:

Sounds good to me, yeah.

Wasmtime GitHub notifications bot (Jun 02 2025 at 22:37):

bank8641 submitted PR review.

Wasmtime GitHub notifications bot (Jun 09 2025 at 18:43):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jun 09 2025 at 18:43):

saulecabrera created PR review comment:

I have part 2/N here https://github.com/bytecodealliance/wasmtime/pull/10989, once that one lands, I think the only remaining piece is to ensure that any scratch register usage in ISA-agnostic goes through the proper path to allocate scratch registers and then we could land this one. Said differently, I think we're 1 change away from unblocking this.

Wasmtime GitHub notifications bot (Jun 09 2025 at 18:44):

saulecabrera edited PR review comment.

Wasmtime GitHub notifications bot (Jun 10 2025 at 14:48):

alexcrichton updated PR #10829.

Wasmtime GitHub notifications bot (Jun 10 2025 at 14:49):

alexcrichton created PR review comment:

I've split out a refactoring from this PR but otherwise the final commit here updates the scratch usage in emit_compute_table_elem_addr which is then will unlock running more tests (yay!)

Wasmtime GitHub notifications bot (Jun 10 2025 at 14:49):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 10 2025 at 15:23):

saulecabrera submitted PR review.

Wasmtime GitHub notifications bot (Jun 10 2025 at 15:23):

saulecabrera created PR review comment:

I've opened the full migration here https://github.com/bytecodealliance/wasmtime/pull/10998, I opened it as draft for now since I still need to amend it to use the newer version of with_scratch and investigate why a disassembly test is failing to compile. I think it should be good after that! I had migrated emit_compute_table_elem_addr as well in that PR, before reading your message here, sorry about that! But the csel changes are still valuable though.

Wasmtime GitHub notifications bot (Jun 10 2025 at 15:25):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jun 10 2025 at 15:25):

alexcrichton created PR review comment:

Nice! And no please use your version, I was just curious to get a feel for things here, I'm not wed to these changes at all

Wasmtime GitHub notifications bot (Jun 10 2025 at 19:22):

alexcrichton updated PR #10829.

Wasmtime GitHub notifications bot (Jun 10 2025 at 19:23):

alexcrichton edited PR #10829:

This commit moves all "ignore these outright" tests for Winch and
AArch64 to "run the test and expect failure" after recent PRs which
removed most of the segfaults/etc which prevented running these tests.
One failure remained which was easy enough to fix here: the cmov
implementation only worked on integer registers, not floating-point
registers, leading to a panic. A helper was added for floating-point selects to
resolve this panic and get some tests passing.

After this fix the ignore method is no longer necessary as all tests
can be run and be flagged as either pass or fail.

Wasmtime GitHub notifications bot (Jun 10 2025 at 19:23):

alexcrichton has marked PR #10829 as ready for review.

Wasmtime GitHub notifications bot (Jun 10 2025 at 19:23):

alexcrichton commented on PR #10829:

Ok now a much smaller PR with all the previous fixes, thanks @saulecabrera! Now this just has a change for fpu_csel plus removing the ignore method, meaning all tests are run in aarch64 right now (although many are still expected to fail)

Wasmtime GitHub notifications bot (Jun 10 2025 at 23:30):

saulecabrera submitted PR review:

Thanks!

Wasmtime GitHub notifications bot (Jun 10 2025 at 23:30):

saulecabrera commented on PR #10829:

all tests are run in aarch64 right now (although many are still expected to fail)

I'm currently looking into it.

Wasmtime GitHub notifications bot (Jun 11 2025 at 00:41):

alexcrichton updated PR #10829.

Wasmtime GitHub notifications bot (Jun 11 2025 at 00:42):

alexcrichton commented on PR #10829:

Reproduced that failure locally and it required a new self.with_aligned_sp during codegen for the wasm unreachable instruction.

Wasmtime GitHub notifications bot (Jun 11 2025 at 00:42):

alexcrichton has enabled auto merge for PR #10829.

Wasmtime GitHub notifications bot (Jun 11 2025 at 00:57):

alexcrichton updated PR #10829.

Wasmtime GitHub notifications bot (Jun 11 2025 at 01:31):

alexcrichton merged PR #10829.

Last updated: Dec 06 2025 at 06:05 UTC