[AVR] Fix rotate instructions

[aykevl]

Summary

This patch fixes some issues with the RORB pseudo instruction.

• A minor issue in which the instructions were said to use the SREG, which is not true.
• An issue with the BLD instruction, which did not have an output operand.
• A major issue in which invalid instructions were generated. The fix also reduce RORB from 4 to 3 instructions, so it's also a small optimization.

These issues were flagged by the machine verifier.

[aykevl]

Oops this is not correct.

[aykevl]

• match avr-gcc output (bst, ror, bld)
• fix issue with the BLD instruction

[benshi001]

Also, there are lint errors, I think this patch is OK if all lint errors can be fixed.

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1349

BLD has a implict SREG operand, do we need to set it as killed?

llvm/lib/Target/AVR/AVRInstrInfo.td

1753

remove this comment line? It is incorrect.

llvm/test/CodeGen/AVR/rot.ll

40

use llvm/utils/update_llc_test_checks.py to update instruction pattern?

[aykevl]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1349

No, the kill flag is optional. See https://youtu.be/objxlZg01D0?t=2379 for details.

Also, I believe setting the kill flag on the SREG does not make a lot of sense, as the SREG is not an allocatable register and in any case it's not true that after that no bits of the SREG register are relevant anymore (it contains the interrupt enable flag, which is always relevant).

llvm/lib/Target/AVR/AVRInstrInfo.td

1753

I have removed it. I think it is meant to say that the AVR rol instruction is an alias and thus the ROLB/RORB instructions are needed, but it is only confusing now.

llvm/test/CodeGen/AVR/rot.ll

40

I could do that, but it would introduce a lot of noise to this patch. That said, I think it would be a good idea to convert most codegen tests to be autogenerated using update_llc_test_checks.py, but perhaps in a separate (non-functional) patch.

[aykevl]

• remove misleading comment
• adjust to fix lint warnings

[benshi001]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1348

Do we need to set dead/kill flags for DstReg in those buildMI ?

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

 bool AVRExpandPseudo::expand<AVR::RORBRd>(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.
 
   MachineInstr &MI = *MBBI;
-  unsigned OpShiftOut, OpLoad, OpShiftIn, OpAdd;
   Register DstReg = MI.getOperand(0).getReg();
-  bool DstIsDead = MI.getOperand(0).isDead();
-  OpShiftOut = AVR::LSRRd;
-  OpLoad = AVR::LDIRdK;
-  OpShiftIn = AVR::RORRd;
-  OpAdd = AVR::ORRdRr;
-
-  // lsr r16
-  // ldi r0, 0
-  // ror r0
-  // or r16, r17
-
-  // Shift out
-  buildMI(MBB, MBBI, OpShiftOut)
-    .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead))
-    .addReg(DstReg);
 
-  // Put 0 in temporary register
-  buildMI(MBB, MBBI, OpLoad)
-    .addReg(SCRATCH_REGISTER, RegState::Define | getDeadRegState(true))
-    .addImm(0x00);
+  // bst r16, 0
+  // ror r16
+  // bld r16, 7
 
-  // Shift in
-  buildMI(MBB, MBBI, OpShiftIn)
-    .addReg(SCRATCH_REGISTER, RegState::Define | getDeadRegState(true))
-    .addReg(SCRATCH_REGISTER);
-
-  // Add the results together using an or-instruction
-  auto MIB = buildMI(MBB, MBBI, OpAdd)
-    .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead))
+  // Move the lowest bit from DstReg into the T bit
+  buildMI(MBB, MBBI, AVR::BST)
     .addReg(DstReg)
-    .addReg(SCRATCH_REGISTER);
+    .addImm(0);
 
-  // SREG is always implicitly killed
-  MIB->getOperand(2).setIsKill();
+  // Rotate to the right
+  buildMI(MBB, MBBI, AVR::RORRd, DstReg)
+    .addReg(DstReg);
+
+  // Move the T bit into the highest bit of DstReg.
+  buildMI(MBB, MBBI, AVR::BLD, DstReg)
+    .addReg(DstReg)
+    .addImm(7);
 
   MI.eraseFromParent();
   return true;
 }
 
 template <>
 bool AVRExpandPseudo::expand<AVR::LSLWRd>(Block &MBB, BlockIt MBBI) {
   MachineInstr &MI = *MBBI;

llvm/lib/Target/AVR/AVRInstrInfo.td

                       "asrw\t$rd",
                       [(set i16:$rd, (AVRasr i16:$src)), (implicit SREG)]>;
 
   def ASRW8Rd : Pseudo<(outs DREGS:$rd),
                        (ins DREGS:$src),
                        "asrw8\t$rd",
                        [(set i16:$rd, (AVRasr8 i16:$src)), (implicit SREG)]>;
 
+  // 8-bit ROL is an alias of ADC Rd, Rd
+
+  def ROLBRd : Pseudo<(outs GPR8:$rd),
+                      (ins GPR8:$src),
+                      "rolb\t$rd",
+                      [(set i8:$rd, (AVRrol i8:$src)), (implicit SREG)]>;
+
+  def RORBRd : Pseudo<(outs GPR8:$rd),
+                      (ins GPR8:$src),
+                      "rorb\t$rd",
+                      [(set i8:$rd, (AVRror i8:$src)), (implicit SREG)]>;
+
   // Bit rotate operations.
   let Uses = [SREG] in
   {
-    // 8-bit ROL is an alias of ADC Rd, Rd
-
-    def ROLBRd : Pseudo<(outs GPR8:$rd),
-                        (ins GPR8:$src),
-                        "rolb\t$rd",
-                        [(set i8:$rd, (AVRrol i8:$src)), (implicit SREG)]>;
-
-    def RORBRd : Pseudo<(outs GPR8:$rd),
-                        (ins GPR8:$src),
-                        "rorb\t$rd",
-                        [(set i8:$rd, (AVRror i8:$src)), (implicit SREG)]>;
 
     def ROLWRd : Pseudo<(outs DREGS:$rd),
                         (ins DREGS:$src),
                         "rolw\t$rd",
                         [(set i16:$rd, (AVRrol i16:$src)), (implicit SREG)]>;
 
     def RORRd : FRd<0b1001,
                     0b0100111,

 // Status register bit load/store operations.
 let Defs = [SREG] in
 def BST : FRdB<0b01,
                (outs),
                (ins GPR8:$rd, i8imm:$b),
                "bst\t$rd, $b",
                []>;
 
-let Uses = [SREG] in
+let Constraints = "$src = $rd",
+Uses = [SREG] in
 def BLD : FRdB<0b00,
-               (outs),
-               (ins GPR8:$rd, i8imm:$b),
+               (outs GPR8:$rd),
+               (ins GPR8:$src, i8imm:$b),
                "bld\t$rd, $b",
                []>;
 
 def CBR : InstAlias<"cbr\t$rd, $k", (ANDIRdK LD8:$rd, imm_com8:$k), 0>;
 
 // CLR Rd
 // Alias for EOR Rd, Rd
 // -------------

llvm/test/CodeGen/AVR/rot.ll

 ; CHECK-LABEL: ror8:
 define i8 @ror8(i8 %val, i8 %amt) {
   ; CHECK:      andi r22, 7
 
   ; CHECK-NEXT: dec r22
   ; CHECK-NEXT: brmi .LBB1_2
 
 ; CHECK-NEXT: .LBB1_1:
-  ; CHECK-NEXT: lsr r24
-  ; CHECK-NEXT: ldi r0, 0
-  ; CHECK-NEXT: ror r0
-  ; CHECK-NEXT: or r24, r0
+  ; CHECK-NEXT: bst r24, 0
+  ; CHECK-NEXT: ror r24
+  ; CHECK-NEXT: bld r24, 7
   ; CHECK-NEXT: dec r22
   ; CHECK-NEXT: brpl .LBB1_1
 
 ; CHECK-NEXT: .LBB1_2:
   ; CHECK-NEXT: ret
   %mod = urem i8 %amt, 8
 
   %inv = sub i8 8, %mod
   %parta = lshr i8 %val, %mod
   %partb = shl i8 %val, %inv
 
   %rotr = or i8 %parta, %partb
 
   ret i8 %rotr
 }