cranelift: 32bit div_s, rem_u, rem_s for aarch64#9850
Conversation
github-actions
Bot
added
cranelift
MarinPostma
force-pushed
the
i32-idiv-aarch64
branch
from
63ec4cf to
99fa5b2
Compare
MarinPostma
force-pushed
the
i32-idiv-aarch64
branch
from
99fa5b2 to
bf30367
Compare
MarinPostma
force-pushed
the
i32-idiv-aarch64
branch
from
26cc570 to
048e050
Compare
MarinPostma
force-pushed
the
i32-idiv-aarch64
branch
from
1c6ea59 to
ae4061b
Compare
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Looks good, thanks a bunch! Very nice to see the test expectations get shorter. A few thoughts below but nothing major.
| (load_constant64_full $I64 extend n)) | ||
|
|
||
| ;; Fallback for integral 32-bit constants | ||
| (rule -1 (imm (integral_ty ty) extend n) |
There was a problem hiding this comment.
Can we say (fits_in_32 ty) rather than ty here, and make this the higher-priority case (so 64-bit is the fallback)? That seems a little cleaner to me than the implicit "everything not I64 is smaller than 64 bits" here (and less likely to break if we try to do other things like support I128 more fully in the future).
| ALUOp::AddS => 0b00101011_000, | ||
| ALUOp::SubS => 0b01101011_000, | ||
| ALUOp::SDiv => 0b10011010_110, | ||
| ALUOp::SDiv => 0b00011010_110, |
There was a problem hiding this comment.
Can we merge the SDiv and UDiv cases now?
| ;; helpers for udiv: | ||
| ;; Helper for placing a `Value` into a `Reg` and validating that it's nonzero. | ||
| (decl put_nonzero_in_reg (Value) Reg) | ||
| ;; It takes a value and entension type, and perform emits the appropriate checks. |
There was a problem hiding this comment.
s/entension/extension/
s/perform/performs/
| ;; Helper for placing a `Value` into a `Reg` and validating that it's nonzero. | ||
| (decl put_nonzero_in_reg (Value) Reg) | ||
| ;; It takes a value and entension type, and perform emits the appropriate checks. | ||
| ;; TODO: restore spec |
There was a problem hiding this comment.
cc @avanhatt @mmcloughlin -- maybe the first instance of active work on the aarch64 backend that needs to update a spec. I definitely don't think we should block this PR on it (so don't worry about this, @MarinPostma!) but it's worth thinking what our short and medium term approaches will be to this since we're upstreamed but don't have a nice CI-integrated workflow yet -- should we keep a queue of such TODOs somewhere?
There was a problem hiding this comment.
about that: I meant to restore them at some point, but I don't know how to run the verification
There was a problem hiding this comment.
Yep, the integration with the normal dev workflow is still very much an open question; we could have you ramp up on that but I don't think it's at the point that we want to require that of everyone yet.
| let value = match extend_to { | ||
| OperandSize::Size32 => { | ||
| if bits < 32 { | ||
| if *extend == generated_code::ImmExtend::Sign { |
There was a problem hiding this comment.
can we make this a match (with Sign and Zero cases)?
| }, | ||
| OperandSize::Size64 => { | ||
| if bits < 64 { | ||
| if *extend == generated_code::ImmExtend::Sign { |
There was a problem hiding this comment.
Likewise here, and we can pull the if bits < 64 in as a guard on one of the match arms as well.
| if ctx.subsumes_fact_optionals(loaded_fact.as_ref(), result_fact) { | ||
| Ok(()) | ||
| } else { | ||
| dbg!(); |
|
hey @cfallin, fixing a bunch of stuff, that's why I put it in draft, but I'll include your review, as soon I manage to fix the tests :) |
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
2 participants
followup to #9798 where I did the ground work for 32bit division, this PR extends 32bits optimizations to rem_u, rem_s and div_s.
This should close #9766.