Stream: git-wasmtime

Topic: wasmtime / PR #14809 cranelift: Implement 128-bit atomic ...

Wasmtime GitHub notifications bot (Oct 19 2026 at 14:46):

Gelbpunkt opened PR #14809 from Gelbpunkt:s390x-128bit-atomic-rmw to bytecodealliance:main:

Unfortunately, it is currently not possible to implement this in ISLE. I tried, and failed miserably. The implementation for 64-bit can be done in ISLE because it can hardcode two physical GPRs - r0 and r1 - which are excluded from regalloc. This is not the case with the VRs, which we are using for the 128-bit integers, so we end up violating SSA rules if we attempt to implement this in ISLE (previous attempt: https://github.com/Gelbpunkt/wasmtime/commit/124d0212ff470557cf5b6fc2aa107daa1d878edc).

On a technical level: We use a CAS loop for this, just like with the 64-bit atomics. It is actually quite straightforward to implement, however there's a small caveat: We cannot use the vector min/max operations prior to z17, so we need to implement a fallback for the baseline if VXRS_EXT3 is not available.

I've tested this in qemu-user with and without VXRS_EXT3, and the runtest passes in both environments.

cc @theotherjimmy @uweigand

Wasmtime GitHub notifications bot (Oct 19 2026 at 14:46):

Gelbpunkt requested wasmtime-compiler-s390x-reviewers for a review on PR #14809.

Wasmtime GitHub notifications bot (Oct 19 2026 at 14:46):

Gelbpunkt requested wasmtime-compiler-reviewers for a review on PR #14809.

Wasmtime GitHub notifications bot (Oct 19 2026 at 14:46):

Gelbpunkt requested cfallin for a review on PR #14809.

Wasmtime GitHub notifications bot (Oct 19 2026 at 14:51):

:memo: bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2026 at 14:51):

:speech_balloon: bjorn3 created PR review comment:

On arm64 this needs to actually be 16 byte aligned I believe. You could do something similar to https://github.com/rust-lang/rustc_codegen_cranelift/blob/8706691e403263efd4216f075caef2b189f3338e/src/common.rs#L397-L409 (basically double the stack slot size and then realign at runtime)

Wasmtime GitHub notifications bot (Oct 19 2026 at 14:59):

Gelbpunkt updated PR #14809.

Wasmtime GitHub notifications bot (Oct 19 2026 at 15:41):

Gelbpunkt updated PR #14809.

Wasmtime GitHub notifications bot (Oct 19 2026 at 15:41):

:memo: Gelbpunkt submitted PR review.

Wasmtime GitHub notifications bot (Oct 19 2026 at 15:41):

:speech_balloon: Gelbpunkt created PR review comment:

Done, it's a bit ugly but works

Wasmtime GitHub notifications bot (Oct 19 2026 at 15:44):

Gelbpunkt commented on PR #14809:

Runtests for s390x in CI are throwing SIGILL in qemu, that's weird. They pass for me locally in qemu-user and also natively on z15. Is the QEMU in CI just too old or what's going on here? :thinking:

Wasmtime GitHub notifications bot (Oct 19 2026 at 17:45):

github-actions[bot] added the label cranelift on PR #14809.

Wasmtime GitHub notifications bot (Oct 19 2026 at 17:45):

github-actions[bot] added the label cranelift:area:machinst on PR #14809.

Wasmtime GitHub notifications bot (Oct 19 2026 at 19:06):

Gelbpunkt edited a comment on PR #14809:

Runtests for s390x in CI are throwing SIGILL in qemu, that's weird. They pass for me locally in qemu-user and also natively on z15. Is the QEMU in CI just too old or what's going on here? :thinking:

Didn't happen on the re-run, but either way probably a QEMU issue

Wasmtime GitHub notifications bot (Oct 20 2026 at 14:25):

bjorn3 commented on PR #14809:

I've tested this with the rust libcore test suite as well as the portable-atomic test suite with CARGO_CFG_SANITIZE=thread to make it the atomic intrinsics rather than inline asm.

Wasmtime GitHub notifications bot (Oct 20 2026 at 14:29):

:memo: uweigand submitted PR review:

Unfortunately, it is currently not possible to implement this in ISLE. I tried, and failed miserably. The implementation for 64-bit can be done in ISLE because it can hardcode two physical GPRs - r0 and r1 - which are excluded from regalloc. This is not the case with the VRs, which we are using for the 128-bit integers, so we end up violating SSA rules if we attempt to implement this in ISLE (previous attempt: https://github.com/Gelbpunkt/wasmtime/commit/124d0212ff470557cf5b6fc2aa107daa1d878edc).

I think it might be possible to extend the Loop construct with a list of temporary registers that are allocated as fixed early-def, and then use registers from that list as scratch registers inside the loop.

However, I'm not sure it's really worth the effort at this point. I added Loop primarily to allow more concise encoding of the various sub-word CAS loops. For the i128 case, implementing it in the emitter seems straightforward enough.

Generally, this implementation looks good to me, but I think we could be more frugal with scratch register use, see inline comments.

Wasmtime GitHub notifications bot (Oct 20 2026 at 14:29):

:speech_balloon: uweigand created PR review comment:

This is really unfortunate. If we don't need scratch1 and scratch2, will this already fix this? The scratch3 and scratch4 must be hard-coded anyway, so maybe we don't need them in the insn either ...

Wasmtime GitHub notifications bot (Oct 20 2026 at 14:29):

:speech_balloon: uweigand created PR review comment:

I don't think we need scratch1 or scratch2 here. It should be possible to only use rd to hold any scratch VR values, something along the lines of:

rd <- mem
scratch3 <- rd

loop:
  case XChg:
    scratch4 <- rn

  case arith:
    rd <- Op(rd, rn)
    scratch4 <- rd

  case min/max fallback:
    if (VecInt128[US]CmpHi(tmp: rd, [rd|rn], [rn|rd]))
      scratch4 <- rn
    else
      scratch4 <- scratch3

  scratch3 <- AtomicCas128(scratch3, scratch4)
  rd <- scratch3

  if cas-failed goto loop

Multiple assignments to rd should be fine as it is marked as early-def (and they happen with your version already anyway).

Wasmtime GitHub notifications bot (Oct 20 2026 at 15:15):

Gelbpunkt updated PR #14809.

Wasmtime GitHub notifications bot (Oct 20 2026 at 15:16):

:speech_balloon: Gelbpunkt created PR review comment:

Right! Good catch, I did not even consider this. Updated the PR accordingly

Wasmtime GitHub notifications bot (Oct 20 2026 at 15:16):

:memo: Gelbpunkt submitted PR review.

Wasmtime GitHub notifications bot (Oct 20 2026 at 15:22):

:speech_balloon: Gelbpunkt created PR review comment:

Removing the two scratch variables makes it 48 bytes

Wasmtime GitHub notifications bot (Oct 20 2026 at 15:22):

:memo: Gelbpunkt submitted PR review.

Wasmtime GitHub notifications bot (Oct 20 2026 at 15:23):

Gelbpunkt updated PR #14809.

Wasmtime GitHub notifications bot (Oct 20 2026 at 16:13):

:memo: uweigand submitted PR review.

Wasmtime GitHub notifications bot (Oct 20 2026 at 16:13):

:speech_balloon: uweigand created PR review comment:

OK, I guess we can live with that. Thanks!

Wasmtime GitHub notifications bot (Oct 20 2026 at 16:14):

:thumbs_up: uweigand submitted PR review:

This version LGTM, thanks!

Wasmtime GitHub notifications bot (Oct 20 2026 at 16:15):

uweigand added PR #14809 cranelift: Implement 128-bit atomic RMW for s390x to the merge queue.

Wasmtime GitHub notifications bot (Oct 20 2026 at 16:38):

github-merge-queue[bot] removed PR #14809 cranelift: Implement 128-bit atomic RMW for s390x from the merge queue.

Wasmtime GitHub notifications bot (Oct 20 2026 at 16:46):

uweigand added PR #14809 cranelift: Implement 128-bit atomic RMW for s390x to the merge queue.

Wasmtime GitHub notifications bot (Oct 20 2026 at 17:22):

:check: uweigand merged PR #14809.

Wasmtime GitHub notifications bot (Oct 20 2026 at 17:22):

uweigand removed PR #14809 cranelift: Implement 128-bit atomic RMW for s390x from the merge queue.

Last updated: Oct 31 2026 at 23:03 UTC