Stream: git-wasmtime

Topic: wasmtime / PR #3572 aarch64: Migrate {s,u}{div,rem} to ISLE

Wasmtime GitHub notifications bot (Nov 30 2021 at 18:40):

alexcrichton opened PR #3572 from isle-4-sdiv-udiv-srem-urem to main:

This commit migrates four different instructions at once to ISLE:

• sdiv
• udiv
• srem
• urem

These all share similar codegen and center around the div instruction
to use internally. The main feature of these was to model the manual
traps since the div instruction doesn't trap on overflow, instead
requiring manual checks to adhere to the semantics of the instruction
itself.

While I was here I went ahead and implemented an optimization for these
instructions when the right-hand-side is a constant with a known value.
For udiv, srem, and urem if the right-hand-side is a nonzero
constant then the checks for traps can be skipped entirely. For sdiv
if the constant is not 0 and not -1 then additionally all checks can be
elided. Finally if the right-hand-side of sdiv is -1 the zero-check is
elided, but it still needs a check for i64::MIN on the left-hand-side
and currently there's a TODO where -1 is still checked too.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Nov 30 2021 at 18:45):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2021 at 18:45):

cfallin created PR review comment:

Just a drive-by comment (I'm happy to review the whole thing too but wanted to note this first): eek, a language wart appears! I had included #t and #f bool constants (Scheme-style) but they lower to 1 / 0 (i.e., integers); this is largely because the DSL compiler's IR has ConstInt but not ConstBool because... no good reason. I see here you've used the $ syntax to just pass through false as an opaque constant name; that works but we should really fix this.

(Outside the scope of this PR obviously, just noting -- I'll file an issue for it. Sorry about this!)

Wasmtime GitHub notifications bot (Nov 30 2021 at 18:47):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2021 at 18:47):

alexcrichton created PR review comment:

Ah ok! I'm happy to switch to whatever is the right way to do this, I think there's other places that stem from https://github.com/bytecodealliance/wasmtime/blob/0580c84405cb5808518b187d1851dc17e7538108/cranelift/codegen/src/prelude.isle#L13-L14 which would need an update as well.

Wasmtime GitHub notifications bot (Nov 30 2021 at 18:51):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Nov 30 2021 at 18:51):

cfallin created PR review comment:

Filed #3573 for this, happy to do the cleanup later; this bit can land as-is for now :-)

Wasmtime GitHub notifications bot (Dec 13 2021 at 16:23):

alexcrichton requested fitzgen for a review on PR #3572.

Wasmtime GitHub notifications bot (Dec 13 2021 at 17:23):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Dec 13 2021 at 17:23):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Dec 13 2021 at 17:23):

fitzgen created PR review comment:

;; Note that aarch64's `udiv` doesn't trap so to respect the semantics of CLIF's `udiv` the check

Wasmtime GitHub notifications bot (Dec 13 2021 at 17:23):

fitzgen created PR review comment:

It is a little strange that trap_if_zero unconditionally uses the trap_code_division_by_zero trap code. trap_if_zero's name doesn't suggest anything about division. Could we either workshop the name into something like trap_if_zero_divisor or make it take a trap code parameter?

Wasmtime GitHub notifications bot (Dec 13 2021 at 17:23):

fitzgen created PR review comment:

Should CondBrKind be moved into ISLE? This seems like a lot of code to write to glue the non-ISLE definition in with the ISLE code. Doesn't need to happen in this PR (or at all) unless you want, but just something I though of when looking at this code.

Wasmtime GitHub notifications bot (Dec 13 2021 at 22:38):

alexcrichton updated PR #3572 from isle-4-sdiv-udiv-srem-urem to main.

Wasmtime GitHub notifications bot (Dec 13 2021 at 22:39):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 13 2021 at 22:39):

alexcrichton created PR review comment:

Currently CondBrKind is pretty heavily used it looks like in the backend and since ISLE can only generate named-field enums instead of tuple-enums like CondBrKind is today I think I'll leave it defined externally, but I agree this'd be a good future cleanup!

Wasmtime GitHub notifications bot (Dec 13 2021 at 23:27):

alexcrichton merged PR #3572.

Last updated: Nov 22 2024 at 16:03 UTC