feat: support i64::checked_mod#1196
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
mooori
force-pushed
the
mooori/checked-mod-i64
branch
from
a279fa6 to
2c4a9b3
Compare
greenhat
left a comment
greenhat
left a comment
There was a problem hiding this comment.
Looking good overall! Please check my note.
Also, IIRC we have arith.smod. Should we use it here?
| |(a, b): &(i64, i64)| { | ||
| if *b == 0 { | ||
| Err(TrapExpectation::DivideByZero) | ||
| } else if *a == i64::MIN && *b == -1 { |
There was a problem hiding this comment.
WebAssembly signed remainder is only undefined for a zero divisor; i64.rem_s(i64::MIN, -1) should return 0, not trap (WebAssembly numerics, irem_s).
There was a problem hiding this comment.
i64.rem_s(i64::MIN, -1) should return 0, not trap
I think this means we have a bug in the lowering of Wasm's i64.rem_s which tests do not catch:
- Wasm
i64.rem_sis probably compiled toi64::checked_modintrinsic - For
(i64::MIN, -1)Wasm returns 0 but the intrinsic traps
The arith tests i64::checked_rem and i64::overflowing_rem do hit the intrinsic i64::checked_mod. Though when compiling i64::checked_rem, i64::overflowing_rem from Rust to Wasm, the generated Wasm handles edge cases explicitly, thus hiding the bug.
My plan is:
- Add test(s) to check whether the
i64.rem_slowering is fine or not - Fix it if needed. Maybe by adding intrinsic
i64::wrapping_modand loweringi64.rem_sto that.
The same issue should then also exist for i32.rem_s and the i32::checked_mod intrinsic.
| # Computes `a % b`, asserting that both inputs are valid i64 values (u32 limbs). | ||
| # | ||
| # Execution traps on division by zero, and on `i64::MIN % -1` (the underlying division overflows). | ||
| pub proc checked_mod # [b_lo, b_hi, a_lo, a_hi] |
There was a problem hiding this comment.
We should add type signatures to all new intrinsics
|
Reverted to draft to work on #1206 first. |
3 participants
i64::checked_modchecked_rem, see feat: supporti32::checked_mod#1174 for the reasoningchecked_mod_i64Closes #1000 and unlocks
i64::overflowing_rem,i64::checked_rem.