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.
Wasmtime GitHub notifications bot (Mar 25 2026 at 19:51):theotherjimmy updated PR #12523.
Wasmtime GitHub notifications bot (Mar 25 2026 at 19:51):theotherjimmy commented on PR #12523:
Rebased for the new types-in-lower-instructions update
Wasmtime GitHub notifications bot (Mar 26 2026 at 11:20):uweigand created PR review comment:
So with this change, we can never generate MSR/MSGR at all anymore. It seems confusing to leave the code emitting them in. I think we should either:
- remove Mul32/Mul64 from AluRR; or
- add new opcodes Mul32CC/Mul64CC or so in AluRRR, which can be used whenever we require the CC-setting variant of multiply.
Wasmtime GitHub notifications bot (Mar 26 2026 at 11:20):uweigand created PR review comment:
Typo "conditons"
Wasmtime GitHub notifications bot (Mar 26 2026 at 11:20):uweigand created PR review comment:
This comment still applies. But this is not an issue that would prevent merging the patch in my opinion, it's just a possible clean-up.
Wasmtime GitHub notifications bot (Mar 26 2026 at 11:20):uweigand submitted PR review:
Sorry for the delay in reviewing. This all looks functionally correct to me now, see a few additional cosmetic comments inline.
Also, I think you should also enable the uadd_overflow_128.clif runtest test case.
Wasmtime GitHub notifications bot (Mar 26 2026 at 11:20):uweigand created PR review comment:
Typo "itegers". Also, I think this comment and the comment above
type_shift_upcan be combined into a single comment - they're really about the same thing.
Wasmtime GitHub notifications bot (Mar 26 2026 at 11:20):uweigand created PR review comment:
Or -final option- just don't use helpers and inline the code, as already done for smul_overflow.
Wasmtime GitHub notifications bot (Mar 26 2026 at 11:20):uweigand created PR review comment:
But even if we keep the helper functions in here, please at least group all the helpers in a separate section ahead of the actual lowering implementations.
Wasmtime GitHub notifications bot (Mar 26 2026 at 18:35):theotherjimmy created PR review comment:
I'll add the Mul*CC to handle that.
Wasmtime GitHub notifications bot (Mar 26 2026 at 18:35):theotherjimmy submitted PR review.
Wasmtime GitHub notifications bot (Mar 26 2026 at 19:47):theotherjimmy submitted PR review.
Wasmtime GitHub notifications bot (Mar 26 2026 at 19:47):theotherjimmy created PR review comment:
smul_overflow is a special case here, as only the lhs is shifted and the umul_overflow does not use condition code conversion, so it's the only lowering using that specific code pattern. For add/sub, rhs is also shifted, so I think the abstraction might be helpful, So I'll move it to inst.isle
Wasmtime GitHub notifications bot (Mar 26 2026 at 20:01):theotherjimmy updated PR #12523.
Wasmtime GitHub notifications bot (Mar 26 2026 at 20:01):theotherjimmy commented on PR #12523:
I've handled most of the review locally. I'll be adding the uadd overflow i128 and verifying locally before pushing that up.
Wasmtime GitHub notifications bot (Apr 01 2026 at 09:18):uweigand submitted PR review:
This now looks good to me, assuming the uadd-overflow-i128 test case can be added successfully. Thanks!
Wasmtime GitHub notifications bot (Apr 01 2026 at 09:18):uweigand created PR review comment:
Missing newline here.
Wasmtime GitHub notifications bot (Apr 09 2026 at 15:54):theotherjimmy requested wasmtime-compiler-s390x-reviewers for a review on PR #12523.
Wasmtime GitHub notifications bot (Apr 09 2026 at 15:54):theotherjimmy updated PR #12523.
Wasmtime GitHub notifications bot (Apr 09 2026 at 15:54):theotherjimmy commented on PR #12523:
uadd_overflow i128 test is passing.
Wasmtime GitHub notifications bot (Apr 09 2026 at 17:05):uweigand submitted PR review:
LGTM now, thanks!
Wasmtime GitHub notifications bot (Apr 09 2026 at 17:05):uweigand added PR #12523 S390x: implement arith overflow to the merge queue.
Wasmtime GitHub notifications bot (Apr 09 2026 at 17:38):uweigand merged PR #12523.
Wasmtime GitHub notifications bot (Apr 09 2026 at 17:38):uweigand removed PR #12523 S390x: implement arith overflow from the merge queue.
Last updated: Apr 12 2026 at 23:10 UTC