aarch64: Migrate {s,u}{div,rem} to ISLE#3572
aarch64: Migrate {s,u}{div,rem} to ISLE#3572alexcrichton merged 2 commits intobytecodealliance:mainfrom
Conversation
| (_2 Unit (emit (MInst.CCmpImm (size_from_ty ty) | ||
| x | ||
| (u8_into_uimm5 1) | ||
| (nzcv $false $false $false $false) |
There was a problem hiding this 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!)
There was a problem hiding this 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
wasmtime/cranelift/codegen/src/prelude.isle
Lines 13 to 14 in 0580c84
There was a problem hiding this comment.
Filed #3573 for this, happy to do the cleanup later; this bit can land as-is for now :-)
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
fitzgen
left a comment
There was a problem hiding this comment.
LGTM modulo a few comments below.
| fn cond_br_zero(&mut self, reg: Reg) -> CondBrKind { | ||
| CondBrKind::Zero(reg) | ||
| } | ||
|
|
||
| fn cond_br_cond(&mut self, cond: &Cond) -> CondBrKind { | ||
| CondBrKind::Cond(*cond) | ||
| } |
There was a problem hiding this 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.
There was a problem hiding this 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!
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.
c7034ea to
83b6108
Compare
This commit migrates four different instructions at once to ISLE:
sdivudivsremuremThese all share similar codegen and center around the
divinstructionto use internally. The main feature of these was to model the manual
traps since the
divinstruction doesn't trap on overflow, insteadrequiring 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, anduremif the right-hand-side is a nonzeroconstant then the checks for traps can be skipped entirely. For
sdivif the constant is not 0 and not -1 then additionally all checks can be
elided. Finally if the right-hand-side of
sdivis -1 the zero-check iselided, but it still needs a check for
i64::MINon the left-hand-sideand currently there's a TODO where
-1is still checked too.