[AVR] Fix codegen for rotate instructions

Summary:
    This patch introduces the ROLBRd and RORBRd pseudo-instructions,
    which implemenent the "traditional" rotate operations; instead of
    the AVR rotate instructions that use the carry bit.

    The code is not optimized at all. Especially when dealing with
    loops of rotate instructions, this codegen should be improved some
    day.

Related bug: 41358 <https://bugs.llvm.org/show_bug.cgi?id=41358>

//Note//: This is my first submitted patch.

Reviewers: dylanmckay, Jim

Reviewed By: dylanmckay

Subscribers: hiraditya, llvm-commits, dylanmckay, dsprenkels

Tags: #llvm

Patched by dsprenkels (Daan Sprenkels)

Differential Revision: https://reviews.llvm.org/D60365

diff --git a/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp b/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
index 83d0f68..f466c5c 100644 (file)
--- a/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
+++ b/llvm/lib/Target/AVR/AVRExpandPseudoInsts.cpp
@@ -52,6 +52,8 @@ private:
 
   /// The register to be used for temporary storage.
   const unsigned SCRATCH_REGISTER = AVR::R0;
+  /// The register that will always contain zero.
+  const unsigned ZERO_REGISTER = AVR::R1;
   /// The IO address of the status register.
   const unsigned SREG_ADDR = 0x3f;
 
@@ -1243,6 +1245,93 @@ bool AVRExpandPseudo::expand<AVR::POPWRd>(Block &MBB, BlockIt MBBI) {
 }
 
 template <>
+bool AVRExpandPseudo::expand<AVR::ROLBRd>(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 OpShift, OpCarry;
+  unsigned DstReg = MI.getOperand(0).getReg();
+  bool DstIsDead = MI.getOperand(0).isDead();
+  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)
+    .addReg(DstReg);
+
+  // Add the carry bit
+  auto MIB = buildMI(MBB, MBBI, OpCarry)
+    .addReg(DstReg, RegState::Define | getDeadRegState(DstIsDead))
+    .addReg(DstReg)
+    .addReg(ZERO_REGISTER);
+
+  // SREG is always implicitly killed
+  MIB->getOperand(2).setIsKill();
+
+  MI.eraseFromParent();
+  return true;
+}
+
+template <>
+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;
+  unsigned 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);
+
+  // 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))
+    .addReg(DstReg)
+    .addReg(SCRATCH_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;
   unsigned OpLo, OpHi, DstLoReg, DstHiReg;
@@ -1562,6 +1651,8 @@ bool AVRExpandPseudo::expandMI(Block &MBB, BlockIt MBBI) {
     EXPAND(AVR::OUTWARr);
     EXPAND(AVR::PUSHWRr);
     EXPAND(AVR::POPWRd);
+    EXPAND(AVR::ROLBRd);
+    EXPAND(AVR::RORBRd);
     EXPAND(AVR::LSLWRd);
     EXPAND(AVR::LSRWRd);
     EXPAND(AVR::RORWRd);

diff --git a/llvm/lib/Target/AVR/AVRISelLowering.cpp b/llvm/lib/Target/AVR/AVRISelLowering.cpp
index f12c59b..4dd843d 100644 (file)
--- a/llvm/lib/Target/AVR/AVRISelLowering.cpp
+++ b/llvm/lib/Target/AVR/AVRISelLowering.cpp
@@ -1472,16 +1472,15 @@ MachineBasicBlock *AVRTargetLowering::insertShift(MachineInstr &MI,
     RC = &AVR::DREGSRegClass;
     break;
   case AVR::Rol8:
-    Opc = AVR::ADCRdRr; // ROL is an alias of ADC Rd, Rd
+    Opc = AVR::ROLBRd;
     RC = &AVR::GPR8RegClass;
-    HasRepeatedOperand = true;
     break;
   case AVR::Rol16:
     Opc = AVR::ROLWRd;
     RC = &AVR::DREGSRegClass;
     break;
   case AVR::Ror8:
-    Opc = AVR::RORRd;
+    Opc = AVR::RORBRd;
     RC = &AVR::GPR8RegClass;
     break;
   case AVR::Ror16:

diff --git a/llvm/lib/Target/AVR/AVRInstrInfo.td b/llvm/lib/Target/AVR/AVRInstrInfo.td
index caca9b6..3de28ea 100644 (file)
--- a/llvm/lib/Target/AVR/AVRInstrInfo.td
+++ b/llvm/lib/Target/AVR/AVRInstrInfo.td
@@ -1676,6 +1676,16 @@ Defs = [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",
@@ -1686,7 +1696,7 @@ Defs = [SREG] in
                     (outs GPR8:$rd),
                     (ins GPR8:$src),
                     "ror\t$rd",
-                    [(set i8:$rd, (AVRror i8:$src)), (implicit SREG)]>;
+                    []>;
 
     def RORWRd : Pseudo<(outs DREGS:$rd),
                         (ins DREGS:$src),

diff --git a/llvm/test/CodeGen/AVR/rot.ll b/llvm/test/CodeGen/AVR/rot.ll
index a7b77d9..49981aa 100644 (file)
--- a/llvm/test/CodeGen/AVR/rot.ll
+++ b/llvm/test/CodeGen/AVR/rot.ll
@@ -10,7 +10,8 @@ define i8 @rol8(i8 %val, i8 %amt) {
   ; CHECK-NEXT: breq LBB0_2
 
 ; CHECK-NEXT: LBB0_1:
-  ; CHECK-NEXT: rol r24
+  ; CHECK-NEXT: lsl r24
+  ; CHECK-NEXT: adc r24, r1
   ; CHECK-NEXT: subi r22, 1
   ; CHECK-NEXT: brne LBB0_1
 
@@ -36,7 +37,10 @@ define i8 @ror8(i8 %val, i8 %amt) {
   ; CHECK-NEXT: breq LBB1_2
 
 ; CHECK-NEXT: LBB1_1:
-  ; CHECK-NEXT: ror r24
+  ; CHECK-NEXT: lsr r24
+  ; CHECK-NEXT: ldi r0, 0
+  ; CHECK-NEXT: ror r0
+  ; CHECK-NEXT: or r24, r0
   ; CHECK-NEXT: subi r22, 1
   ; CHECK-NEXT: brne LBB1_1