Stream: git-wasmtime

Topic: wasmtime / PR #9541 ISLE: aarch64: fix narrow sdiv on int...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2024 at 16:54):

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 for intmin 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-bit intmin - 1 does not overflow with a 32-bit ARM instruction. This PR adds a left-shift of the dividend before feeding into the intmin 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 of sdiv. 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 the aarch64 assembler.

This PR also adds a file test for the new instruction sequence.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2024 at 16:54):

avanhatt requested cfallin for a review on PR #9541.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2024 at 16:54):

avanhatt requested wasmtime-compiler-reviewers for a review on PR #9541.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2024 at 16:58):

avanhatt updated PR #9541.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2024 at 16:59):

avanhatt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2024 at 16:59):

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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2024 at 17:00):

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?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2024 at 17:00):

avanhatt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2024 at 17:53):

cfallin submitted PR review:

Looks great, thanks!

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2024 at 17:53):

cfallin created PR review comment:

Sure -- no problem to re-add this when the rest is upstreamed.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 01 2024 at 18:22):

cfallin merged PR #9541.

Last updated: Nov 22 2024 at 17:03 UTC