avanhatt opened PR #9541 from avanhatt:narrow_sdiv_aarch64
to bytecodealliance:main
:
Fixes https://github.com/bytecodealliance/wasmtime/issues/9537
Clif's semantics (inherited from Wasm) specify are that divides should trap on
intmin/-1
. The manual check forintmin
in this case in ISLE is to add 1 to the dividend and check for overflow, however, the rule was not previously catching that an 8- or 16-bitintmin - 1
does not overflow with a 32-bit ARM instruction. This PR adds a left-shift of the dividend before feeding into theintmin
check, such that e.g. 0x00000080 is checked as 0x80000000. A ported version of this has been verified as correct by our prototype.Note this adds 1 additional instruction (
lsl
) in the 8- and 16-bit cases ofsdiv
. An alternative would be to check against hard-coded constants for those cases, but we think this would require adding a new conditional compare to theaarch64
assembler.This PR also adds a file test for the new instruction sequence.
avanhatt requested cfallin for a review on PR #9541.
avanhatt requested wasmtime-compiler-reviewers for a review on PR #9541.
avanhatt updated PR #9541.
avanhatt submitted PR review.
avanhatt created PR review comment:
I added this because this same pattern is used enough that a helper seems warranted (
cls
,clz
,bitrev
) but saving changing those for another PR.
avanhatt created PR review comment:
This spec is now stale, but the changes needed to verify the code correctly are ones we still need to upstream. Just removing for now?
avanhatt submitted PR review.
cfallin submitted PR review:
Looks great, thanks!
cfallin created PR review comment:
Sure -- no problem to re-add this when the rest is upstreamed.
cfallin merged PR #9541.
Last updated: Dec 23 2024 at 12:05 UTC