[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

1361

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

41–42

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

[aykevl]

llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp

1361

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

41–42

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
+  // ror r1
+  // clr r1
 
   // Shift out
-  buildMI(MBB, MBBI, OpShiftOut)
+  buildMI(MBB, MBBI, AVR::LSRRd)
     .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);
-
-  // Shift in
-  buildMI(MBB, MBBI, OpShiftIn)
-    .addReg(SCRATCH_REGISTER, RegState::Define | getDeadRegState(true))
-    .addReg(SCRATCH_REGISTER);
+  // Shift in, clobbering the zero register
+  buildMI(MBB, MBBI, AVR::RORRd)
+    .addReg(ZERO_REGISTER, RegState::Define | getDeadRegState(true))
+    .addReg(ZERO_REGISTER);
 
-  // Add the results together using an or-instruction
-  auto MIB = buildMI(MBB, MBBI, OpAdd)
-    .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead))
-    .addReg(DstReg)
-    .addReg(SCRATCH_REGISTER);
+  // Clear the zero register
+  auto MIB = buildMI(MBB, MBBI, AVR::EORRdRr, ZERO_REGISTER)
+    .addReg(ZERO_REGISTER)
+    .addReg(ZERO_REGISTER);
 
   // SREG is always implicitly killed
   MIB->getOperand(2).setIsKill();
 
   MI.eraseFromParent();
   return true;
 }
 
 template <>
 bool AVRExpandPseudo::expand<AVR::LSLWRd>(Block &MBB, BlockIt MBBI) {
   MachineInstr &MI = *MBBI;
   Register DstLoReg, DstHiReg;
   Register DstReg = MI.getOperand(0).getReg();
   bool DstIsDead = MI.getOperand(0).isDead();
   bool DstIsKill = MI.getOperand(1).isKill();
   bool ImpIsDead = MI.getOperand(2).isDead();
   unsigned OpLo = AVR::ADDRdRr; // ADD Rd, Rd <==> LSL Rd
   unsigned OpHi = AVR::ADCRdRr; // ADC Rd, Rd <==> ROL Rd

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,

llvm/test/CodeGen/AVR/rot.ll

 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: ror r1
+  ; CHECK-NEXT: clr r1
   ; 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
 }