[PATCH] D152248: [AVR] Fix incorrect expanded pseudo instruction ROLBRd
Ayke via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 6 06:19:27 PDT 2023
aykevl added a comment.
This looks correct at first glance, but of course a bit inefficient.
I see two options to make this efficient:
1. As proposed by @Patryk27, using two different instructions to differentiate between `R1` and `R17`. This most likely works just fine.
2. Custom lowering for ROLB that adds the zero register. There are many custom lowered instructions, this could be one of them.
I have a slight preference for option 1 (as it's perhaps a little bit more explicit), but either way would work.
================
Comment at: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp:1482
+ // xor r0, r0
// add r16, r16
----------------
Nit: this is `eor`, not `xor`.
================
Comment at: llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp:1488
+ // due to aykevl's comment in https://reviews.llvm.org/D152063 .
+ buildMI(MBB, MBBI, AVR::EORRdRr)
+ .addReg(TmpReg, RegState::Define)
----------------
Patryk27 wrote:
> Patryk27 wrote:
> > benshi001 wrote:
> > > Patryk27 wrote:
> > > > Hmm, this feels kinda excessive - wouldn't adding `let Uses = [R1]` to `ROLBRd` be sufficient?
> > > >
> > > > ```
> > > > let Uses = [R1] in
> > > > def ROLBRd : Pseudo<(outs GPR8
> > > > : $rd),
> > > > (ins GPR8
> > > > : $src),
> > > > "rolb\t$rd",
> > > > [(set i8
> > > > : $rd, (AVRrol i8
> > > > : $src)),
> > > > (implicit SREG)]>;
> > > > ```
> > > Because it maybe `R17` on AVRTINY.
> > ah, true, true
> Maybe we could have two separate instructions, then?
>
> Something like:
>
> ```
> diff --git a/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp b/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
> index 726ed7303746..94cabb4e1387 100644
> --- a/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
> +++ b/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
> @@ -1481,7 +1481,48 @@ bool AVRExpandPseudo::expand<AVR::POPWRd>(Block &MBB, BlockIt MBBI) {
> }
>
> template <>
> -bool AVRExpandPseudo::expand<AVR::ROLBRd>(Block &MBB, BlockIt MBBI) {
> +bool AVRExpandPseudo::expand<AVR::ROLBRdNT>(Block &MBB, BlockIt MBBI) {
> + // In AVR, the rotate instructions behave quite unintuitively. They rotate
> + // bits through the carry bit in SREG, effectively rotating over 9 bits,
> + // instead of 8. This is useful when we are dealing with numbers over
> + // multiple registers, but when we actually need to rotate stuff, we have
> + // to explicitly add the carry bit.
> +
> + const AVRSubtarget &STI = MBB.getParent()->getSubtarget<AVRSubtarget>();
> +
> + MachineInstr &MI = *MBBI;
> + unsigned OpShift, OpCarry;
> + Register DstReg = MI.getOperand(0).getReg();
> + Register ZeroReg = STI.getZeroRegister();
> + bool DstIsDead = MI.getOperand(0).isDead();
> + bool DstIsKill = MI.getOperand(1).isKill();
> + OpShift = AVR::ADDRdRr;
> + OpCarry = AVR::ADCRdRr;
> +
> + // add r16, r16
> + // adc r16, r1
> +
> + // Shift part
> + buildMI(MBB, MBBI, OpShift)
> + .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead))
> + .addReg(DstReg, RegState::Kill)
> + .addReg(DstReg, RegState::Kill);
> +
> + // Add the carry bit
> + auto MIB = buildMI(MBB, MBBI, OpCarry)
> + .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead))
> + .addReg(DstReg, getKillRegState(DstIsKill))
> + .addReg(ZeroReg);
> +
> + MIB->getOperand(3).setIsDead(); // SREG is always dead
> + MIB->getOperand(4).setIsKill(); // SREG is always implicitly killed
> +
> + MI.eraseFromParent();
> + return true;
> +}
> +
> +template <>
> +bool AVRExpandPseudo::expand<AVR::ROLBRdT>(Block &MBB, BlockIt MBBI) {
> // In AVR, the rotate instructions behave quite unintuitively. They rotate
> // bits through the carry bit in SREG, effectively rotating over 9 bits,
> // instead of 8. This is useful when we are dealing with numbers over
> @@ -2605,7 +2646,8 @@ bool AVRExpandPseudo::expandMI(Block &MBB, BlockIt MBBI) {
> EXPAND(AVR::OUTWARr);
> EXPAND(AVR::PUSHWRr);
> EXPAND(AVR::POPWRd);
> - EXPAND(AVR::ROLBRd);
> + EXPAND(AVR::ROLBRdNT);
> + EXPAND(AVR::ROLBRdT);
> EXPAND(AVR::RORBRd);
> EXPAND(AVR::LSLWRd);
> EXPAND(AVR::LSRWRd);
> diff --git a/llvm/lib/Target/AVR/AVRISelLowering.cpp b/llvm/lib/Target/AVR/AVRISelLowering.cpp
> index e44ef51ab3a8..d97b02681f2e 100644
> --- a/llvm/lib/Target/AVR/AVRISelLowering.cpp
> +++ b/llvm/lib/Target/AVR/AVRISelLowering.cpp
> @@ -1758,6 +1758,7 @@ MachineBasicBlock *AVRTargetLowering::insertShift(MachineInstr &MI,
> MachineFunction *F = BB->getParent();
> MachineRegisterInfo &RI = F->getRegInfo();
> const TargetInstrInfo &TII = *Subtarget.getInstrInfo();
> + const AVRSubtarget &STI = F->getSubtarget<AVRSubtarget>();
> DebugLoc dl = MI.getDebugLoc();
>
> switch (MI.getOpcode()) {
> @@ -1789,7 +1790,7 @@ MachineBasicBlock *AVRTargetLowering::insertShift(MachineInstr &MI,
> RC = &AVR::DREGSRegClass;
> break;
> case AVR::Rol8:
> - Opc = AVR::ROLBRd;
> + Opc = STI.hasTinyEncoding() ? AVR::ROLBRdT : AVR::ROLBRdNT;
> RC = &AVR::GPR8RegClass;
> break;
> case AVR::Rol16:
> diff --git a/llvm/lib/Target/AVR/AVRInstrInfo.td b/llvm/lib/Target/AVR/AVRInstrInfo.td
> index d0e75733114a..630369d2d25e 100644
> --- a/llvm/lib/Target/AVR/AVRInstrInfo.td
> +++ b/llvm/lib/Target/AVR/AVRInstrInfo.td
> @@ -2029,7 +2029,8 @@ let Constraints = "$src = $rd", Defs = [SREG] in {
> def ASRWLoRd : Pseudo<(outs DREGS:$rd), (ins DREGS:$src), "asrwlo\t$rd",
> [(set i16:$rd, (AVRasrlo i16:$src)), (implicit SREG)]>;
>
> - def ROLBRd : Pseudo<(outs GPR8
> + let Uses = [R1] in
> + def ROLBRdNT : Pseudo<(outs GPR8
> : $rd),
> (ins GPR8
> : $src),
> @@ -2037,7 +2038,20 @@ let Constraints = "$src = $rd", Defs = [SREG] in {
> [(set i8
> : $rd, (AVRrol i8
> : $src)),
> - (implicit SREG)]>;
> + (implicit SREG)]>,
> + Requires<[HasNonTinyEncoding]>;
> +
> + let Uses = [R17] in
> + def ROLBRdT : Pseudo<(outs GPR8
> + : $rd),
> + (ins GPR8
> + : $src),
> + "rolb\t$rd",
> + [(set i8
> + : $rd, (AVRrol i8
> + : $src)),
> + (implicit SREG)]>,
> + Requires<[HasTinyEncoding]>;
>
> def RORBRd : Pseudo<(outs GPR8
> : $rd),
> ```
>
> Of course, this naive approach copy-pastes the code responsible for expanding the pseudo-instruction, but that could should be pretty easy to de-duplicate.
You can use this:
```
buildMI(MBB, MBBI, AVR::EORRdRr, TmpReg)
.addReg(TmpReg)
.addReg(TmpReg);
```
The third parameter to `buildMI` is the register to define.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D152248/new/
https://reviews.llvm.org/D152248
More information about the llvm-commits
mailing list