Stream: git-wasmtime

Topic: wasmtime / PR #11583 riscv: implement ISLE lowering for u...

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 00:29):

thatrajeevkr opened PR #11583 from thatrajeevkr:fix/riscv-uadd-overflow to bytecodealliance:main:

Implements ISLE lowering for uadd_overflow.i64 in the RISC-V backend, enabling Cranelift to handle unsigned addition with overflow for 64-bit, sub-64-bit, and 128-bit types.

i64: add + sltu to detect overflow

Sub-64-bit types: zero-extend, then add + sltu

i128: multiword add with carry propagation

Fixes unsupported feature error when compiling Wasm modules with large address offsets

Fix #11540

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 00:29):

thatrajeevkr requested cfallin for a review on PR #11583.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 00:29):

thatrajeevkr requested wasmtime-compiler-reviewers for a review on PR #11583.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 09:17):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 09:17):

bjorn3 created PR review comment:

I don't think this would work. rv_sltu does a 64bit unsigned less than comparison, but if you add two 32bit integers into a 64bit register you will never overflow the full 64bit and thus tmp_x + tmp_y will never be less than tmp_x. What LLVM does instead is to use a 32bit add (addw), in which case sltu would work just fine. That way also only x would need to be zero extended I think. https://rust.godbolt.org/z/zz5avj33h

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 10:05):

thatrajeevkr updated PR #11583.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 10:06):

thatrajeevkr submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 10:06):

thatrajeevkr created PR review comment:

Thanks for the input, I have put in a small fix for this.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 10:29):

thatrajeevkr requested bjorn3 for a review on PR #11583.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 16:26):

cfallin submitted PR review:

This looks good -- thanks!

In addition to the little formatting nit below, could you update our runtest for this instruction so that it tests riscv64 too?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 16:26):

cfallin created PR review comment:

Tiny nit, but let's have a blank line between the section header comment and the ;; For i64 ... start of this rule.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 16:42):

thatrajeevkr updated PR #11583.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 16:43):

thatrajeevkr submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 16:43):

thatrajeevkr created PR review comment:

just made this change now!

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 16:53):

thatrajeevkr updated PR #11583.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 17:01):

cfallin submitted PR review:

Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 17:01):

cfallin has enabled auto merge for PR #11583.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 17:20):

cfallin commented on PR #11583:

@thatrajeevkr it looks like the runtest failed because it also contains narrower-than-32-bit cases; in theory you could implement those as well, but probably the easier fix is to split the runtest file into two (keep the preamble and split the test functions and expectations), with 64/32-bit cases separate from 16/8-bit cases (perhaps in a file called uadd_overflow_narrow.clif)?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 17:20):

cfallin edited a comment on PR #11583:

@thatrajeevkr it looks like the runtest failed because it also contains narrower-than-32-bit cases; in theory you could implement those as well, but probably the easier fix is to split the runtest file into two (keep the preamble and split the test functions and expectations), with 128/64/32-bit cases separate from 16/8-bit cases (perhaps in a file called uadd_overflow_narrow.clif) and then enable riscv64 only for 128/64/32 bits?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 20:32):

cfallin has disabled auto merge for PR #11583.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 20:32):

thatrajeevkr updated PR #11583.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 21:04):

cfallin commented on PR #11583:

@thatrajeevkr It looks like an i128 test is failing now, showing overflow when there is none -- mind taking a look?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 21:18):

thatrajeevkr updated PR #11583.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 21:24):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 21:24):

cfallin created PR review comment:

Thinking about this tweak -- I suspect that the second arm of the OR was supposed to cover the case where the high-half sum wraps around back to the original value when carry is set?

That is, normally we can see if x + y overflows by checking if the sum is less than x (has wrapped around). The closest it can get is if y is all-ones (-1), then we get to x - 1; so strict less than (sltu, i.e., set-less-than unsigned) is appropriate.

But for x + y + carry, it's possible that the sum wraps around all the way back to x, so carry && (x + y + carry == x) is the appropriate second case, I think? So overall sum_hi_with_carry < x_hi || (carry && sum_high_with_carry == x_hi)?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 21:29):

thatrajeevkr submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 21:29):

thatrajeevkr created PR review comment:

overflow XReg (rv_or (rv_sltu sum_hi_with_carry x_hi)
                     (rv_and carry (rv_eq sum_hi_with_carry x_hi)))

how about this method?

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 22:01):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 22:01):

cfallin created PR review comment:

That looks right; and I think the remaining failure should cover the case that this handles!

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 22:15):

thatrajeevkr updated PR #11583.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 22:27):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2025 at 22:27):

cfallin created PR review comment:

@thatrajeevkr actually it appears that rv_eq isn't a defined constructor -- see the latest test failures. You should be able to cargo check locally (as that error message states) to iterate until it builds at least, before going through a CI roundtrip. (We also have docs on cross-compiling and testing with qemu if you want to iterate on the runtest locally, but that's much more involved...)

view this post on Zulip Wasmtime GitHub notifications bot (Sep 03 2025 at 13:14):

thatrajeevkr submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 03 2025 at 13:14):

thatrajeevkr created PR review comment:

I found a fix for this which builds fine in my local, I will commit those changes.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 03 2025 at 16:29):

thatrajeevkr updated PR #11583.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 03 2025 at 17:25):

cfallin merged PR #11583.

Last updated: Dec 06 2025 at 06:05 UTC