aarch64: support udiv for 32bit integers #9798
Conversation
MarinPostma
requested review from
cfallin and
alexcrichton
and removed request for
a team
github-actions
bot
added
cranelift
Subscribe to Label Action
This 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
| @@ -870,6 +870,7 @@ | |||
| ;; *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
| @@ -870,6 +870,7 @@ | |||
| ;; *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?
| @@ -870,6 +870,7 @@ | |||
| ;; *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 Action
This 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 |
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.