Wasmtime GitHub notifications bot (Feb 04 2026 at 18:58): Wasmtime GitHub notifications bot (Feb 04 2026 at 18:58):theotherjimmy opened PR #12523 from theotherjimmy:s390x-arith-overflow to bytecodealliance:main:
Implement the following isle instructions for S390x:
uadd_overflowsadd_overflowusub_overflowssub_overflowumul_overflowsmul_overflow
Wasmtime GitHub notifications bot (Feb 04 2026 at 18:58):theotherjimmy requested wasmtime-compiler-reviewers for a review on PR #12523.
Wasmtime GitHub notifications bot (Feb 04 2026 at 18:58):theotherjimmy requested alexcrichton for a review on PR #12523.
Wasmtime GitHub notifications bot (Feb 04 2026 at 19:32):alexcrichton commented on PR #12523:
r? @uweigand
Wasmtime GitHub notifications bot (Feb 04 2026 at 19:53):github-actions[bot] added the label cranelift on PR #12523.
Wasmtime GitHub notifications bot (Feb 05 2026 at 12:51):uweigand created PR review comment:
This is again unnecessary.
Wasmtime GitHub notifications bot (Feb 05 2026 at 12:51):uweigand created PR review comment:
Since you're doing a 64-bit multiply here, you still need to zero-extend the inputs even here. Or else, you could use a 32x32->64 unsigned multiply.
Wasmtime GitHub notifications bot (Feb 05 2026 at 12:51):uweigand submitted PR review.
Wasmtime GitHub notifications bot (Feb 05 2026 at 12:51):uweigand created PR review comment:
The inputs are not guaranteed to be zero-extended, so you have to do that here.
Wasmtime GitHub notifications bot (Feb 05 2026 at 12:51):uweigand created PR review comment:
This still needs to do the != 0 conversion to boolean on the high part.
Wasmtime GitHub notifications bot (Feb 05 2026 at 12:51):uweigand created PR review comment:
This should do a != 0 test to set a boolean.
Wasmtime GitHub notifications bot (Feb 05 2026 at 12:51):uweigand created PR review comment:
This is more of a cosmetic issue, but I think I'd prefer to keep these types of helpers in inst.isle - see e.g. what we're doing for the ..._sat_reg helpers. Ideally, we'd have a interface between inst.isle and lower.isle that looks more symmetrical across the different types ...
Wasmtime GitHub notifications bot (Feb 05 2026 at 12:51):uweigand created PR review comment:
This seems just wrong. The second return should be a boolean indicating whether overflow happened, not simply the high part of the widened multiplication.
Wasmtime GitHub notifications bot (Feb 05 2026 at 12:51):uweigand created PR review comment:
For the 32-bit and 64-bit cases of smul_overflow, why don't you simply use MS(G)RKC, which sets CC indicating overflow - that would work exactly the same as for sadd/ssub.
For the 8-bit and 16-bit cases, you should be able to use a similar shift trick as for sadd/ssub, only you have to make sure to shift just one of the inputs (the other must still be sign-extended).
Wasmtime GitHub notifications bot (Feb 05 2026 at 12:51):uweigand created PR review comment:
This is unnecessary - the high bytes are considered don't care.
Wasmtime GitHub notifications bot (Feb 05 2026 at 16:28):theotherjimmy submitted PR review.
Wasmtime GitHub notifications bot (Feb 05 2026 at 16:28):theotherjimmy created PR review comment:
Multiply integers unsigned with overflow out.
ofis set when the multiplication overflowed.
text a &= x * y \pmod 2^B \\ of &= x * y > 2^BMy read is that the overflow out is the whole upper word size of the multiply.
Wasmtime GitHub notifications bot (Feb 05 2026 at 16:28):theotherjimmy submitted PR review.
Wasmtime GitHub notifications bot (Feb 05 2026 at 16:28):theotherjimmy created PR review comment:
Multiply integers unsigned with overflow out.
ofis set when the multiplication overflowed.
text a &= x * y \pmod 2^B \\ of &= x * y > 2^BMy read is that the overflow out is the whole upper word size of the multiply.
Wasmtime GitHub notifications bot (Feb 05 2026 at 16:28):theotherjimmy deleted PR review comment.
Wasmtime GitHub notifications bot (Feb 05 2026 at 16:58):theotherjimmy updated PR #12523.
Wasmtime GitHub notifications bot (Feb 05 2026 at 17:29):theotherjimmy updated PR #12523.
Wasmtime GitHub notifications bot (Feb 05 2026 at 17:39):uweigand submitted PR review.
Wasmtime GitHub notifications bot (Feb 05 2026 at 17:39):uweigand created PR review comment:
This needs to sign-extend from byte, not word.
Wasmtime GitHub notifications bot (Feb 05 2026 at 17:39):uweigand created PR review comment:
This is wrong, the CC checks for signed overflow, not unsigned overflow. For
umul_overflow, you'll have to either multiply in a wider mode, or use a widening multiply, and then check the high part is nonzero to detect unsigned overflow.
Wasmtime GitHub notifications bot (Feb 05 2026 at 17:39):uweigand created PR review comment:
And this needs to sign-extend from halfword.
Wasmtime GitHub notifications bot (Feb 05 2026 at 17:39):uweigand created PR review comment:
This is unfortunately wrong as msr does not set CC. You'll have to use msrkc.
Wasmtime GitHub notifications bot (Feb 05 2026 at 17:39):uweigand created PR review comment:
And msgkrc here.
Wasmtime GitHub notifications bot (Feb 05 2026 at 17:41):theotherjimmy edited PR review comment.
Wasmtime GitHub notifications bot (Feb 05 2026 at 17:41):uweigand commented on PR #12523:
You should enable the run-time filetests on s390x as well, these would probably have caught some of those errors:
filetests/filetests/runtests/overflow.clif
Wasmtime GitHub notifications bot (Feb 05 2026 at 17:42):uweigand edited a comment on PR #12523:
You should enable the run-time filetests on s390x as well, these would probably have caught some of those errors:
filetests/filetests/runtests/*overflow*.clif
Wasmtime GitHub notifications bot (Feb 05 2026 at 17:43):theotherjimmy submitted PR review.
Wasmtime GitHub notifications bot (Feb 05 2026 at 17:43):theotherjimmy created PR review comment:
Ah, oops.
Wasmtime GitHub notifications bot (Feb 05 2026 at 18:16):theotherjimmy updated PR #12523.
Wasmtime GitHub notifications bot (Feb 13 2026 at 21:05):theotherjimmy updated PR #12523.
Wasmtime GitHub notifications bot (Feb 13 2026 at 21:07):theotherjimmy commented on PR #12523:
@uweigand I ran all these tests on my lpar. The current version passes all enabled runtests on s390x, and I enabled the runtests for these instructions.
Wasmtime GitHub notifications bot (Feb 13 2026 at 21:09):theotherjimmy commented on PR #12523:
It looks like I need to rebase onto main. I'll do that now then.
Wasmtime GitHub notifications bot (Feb 13 2026 at 21:11):theotherjimmy updated PR #12523.
Wasmtime GitHub notifications bot (Feb 13 2026 at 21:13):theotherjimmy edited a comment on PR #12523:
It looks like I need to rebase onto main. I'll do that now then. Tests still pass with the rebase onto main.
Wasmtime GitHub notifications bot (Feb 13 2026 at 21:44):theotherjimmy edited a comment on PR #12523:
@uweigand I ran all these tests on my lpar. The current version passes all enabled runtests on s390x, and I enabled the runtests for these instructions.
I missed the
cargo testtests. My bad. Please hold off on reviewing while I fix them.
Wasmtime GitHub notifications bot (Feb 13 2026 at 22:03):theotherjimmy updated PR #12523.
Wasmtime GitHub notifications bot (Feb 13 2026 at 22:04):theotherjimmy commented on PR #12523:
Fixed
cargo testlocally.
Wasmtime GitHub notifications bot (Feb 19 2026 at 16:07):theotherjimmy updated PR #12523.
Wasmtime GitHub notifications bot (Feb 19 2026 at 16:08):theotherjimmy commented on PR #12523:
Rebased for the regalloc2 version bump. Re-blessed all new tests. ran all runtests on my s390x LPAR, they passed.
Wasmtime GitHub notifications bot (Feb 19 2026 at 19:31):alexcrichton requested uweigand for a review on PR #12523.
Last updated: Feb 24 2026 at 04:36 UTC