aarch64: support udiv for 32bit integers#9798
Conversation
MarinPostma
requested review from
alexcrichton and
cfallin
and removed request for
a team
github-actions
Bot
added
cranelift
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "winch"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
MarinPostma
force-pushed
the
i32-div-aarch64
branch
from
33f141b to
9399fcc
Compare
| ;; *execute* the embedded `Inst`. (In the emitted code, we use the inverse | ||
| ;; of this condition in a branch that skips the trap instruction.) | ||
| (TrapIf | ||
| (size OperandSize) |
There was a problem hiding this comment.
I think we mostly had two choices here:
- either make the size part of the
CondBrKind, which is tighter, but yields more changes in the code - or setting the size in the op itself (current approach), which required less changes
There was a problem hiding this comment.
I think I might prefer the first option actually -- I was a bit confused at first how the OperandSize would affect the TrapIf if we're just talking about conditional branch kinds like b.lo, b.hi, etc; until I realized that CondBrKind::Zero(reg) and NonZero(reg) exist and encode cbz/cbnz. If you don't mind, do you think you could try the refactor out? On the upside, it seems like it should get rid of a bunch of extraneous OperandSizes on branches emitted in various places that are condcode-based (e.g. in abi.rs above and in many of the emit-tests below).
There was a problem hiding this comment.
Also, separately, if we modify CondBrKind, this is toward the direction that we want for supporting 32-bit zero/nonzero tests in regular conditional branches too. We don't have to implement that now (you can unimplemented!() it in the CondBr case; but if you wanted to, it would certainly be fine with me) but it's nice to lay the groundwork.
There was a problem hiding this comment.
This make sense to me, I'll refactor
MarinPostma
force-pushed
the
i32-div-aarch64
branch
from
9399fcc to
5ec6016
Compare
github-actions
Bot
added
the
cranelift:area:machinst
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks! One thought below on the way that TrapIf is extended, but generally I like how this is turning out.
| ;; *execute* the embedded `Inst`. (In the emitted code, we use the inverse | ||
| ;; of this condition in a branch that skips the trap instruction.) | ||
| (TrapIf | ||
| (size OperandSize) |
There was a problem hiding this comment.
I think I might prefer the first option actually -- I was a bit confused at first how the OperandSize would affect the TrapIf if we're just talking about conditional branch kinds like b.lo, b.hi, etc; until I realized that CondBrKind::Zero(reg) and NonZero(reg) exist and encode cbz/cbnz. If you don't mind, do you think you could try the refactor out? On the upside, it seems like it should get rid of a bunch of extraneous OperandSizes on branches emitted in various places that are condcode-based (e.g. in abi.rs above and in many of the emit-tests below).
| fn show_reg_sized(reg: Reg, size: OperandSize) -> String { | ||
| match reg.class() { | ||
| RegClass::Int => show_ireg_sized(reg, size), | ||
| RegClass::Float => todo!(), |
There was a problem hiding this comment.
todo!() here -- can we just show the reg (and below for vector too) with show_reg for now?
| ;; *execute* the embedded `Inst`. (In the emitted code, we use the inverse | ||
| ;; of this condition in a branch that skips the trap instruction.) | ||
| (TrapIf | ||
| (size OperandSize) |
There was a problem hiding this comment.
Also, separately, if we modify CondBrKind, this is toward the direction that we want for supporting 32-bit zero/nonzero tests in regular conditional branches too. We don't have to implement that now (you can unimplemented!() it in the CondBr case; but if you wanted to, it would certainly be fine with me) but it's nice to lay the groundwork.
|
Thanks @cfallin, will do the refactor 👍 |
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift:area:machinst", "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
This PR takes first steps towards completing #9766, by implementing support for 32bit unsigned division in cranelift and winch.
we had to take the following steps to enable that:
trap_ifsize aware: this avoids having to perform extensions beforecbz, shaving more instructions. This is reflected in the lowering rules for checking division by zero for udiv.