[AVR] Optimize 16-bit ORs with '0'

[anmol]

Summary

Fixes PR 31344

[dylanmckay]

Looking really nice! Added a few nits

lib/Target/AVR/AVRExpandPseudoInsts.cpp

77

As this method doesn't actually do any expansion and only returns a boolean, you should rename it to something like isLogicImmOpRedundant.

Also, you should add a const qualifier to the method.

225

This temporary variable is unnecessary, plus it would be clearer if it looked if (expandLogicImmNeeded(..) { }

[anmol]

Thank you, for the review.

• Renamed AVRExpandPseudo::expandLogicImmNeeded() to AVRExpandPseudo::isLogicImmOpRedundant()
• AVRExpandPseudo::isLogicImmOpRedundant() is now const qualified.
• Eliminated the temporary, so that the check for redundancy appears in the if's conditional clause. PS: The sense of the check is now inverted relative to the prior version.

[dylanmckay]

Looks good!

[anmol]

In D28121#631960, @dylanmckay wrote:

Looks good!

Thank you! What are the next steps to follow? How does it get into the mainline trunk sources? Do we change state to 'RESOLVED' once it does?

[fhahn]

Somebody with commit access has to commit your change (with attribution) and Phabricator will close the review afterwards. PR 31344 needs to be closed manually afterwards.

If you do not have commit access it is best to ask somebody to commit your changes.

[anmol]

In D28121#632466, @fhahn wrote:

Somebody with commit access has to commit your change (with attribution) and Phabricator will close the review afterwards. PR 31344 needs to be closed manually afterwards.

If you do not have commit access it is best to ask somebody to commit your changes.

Thank you, for helping me understand the process. Perhaps Dylan has commit access; let's wait to hear back from him.

[dylanmckay]

Committed in r290732!

[anmol]

In D28121#632556, @dylanmckay wrote:

Committed in r290732!

Thank you!

lib/Target/AVR/AVRExpandPseudoInsts.cpp

MachineInstrBuilder buildMI(Block &MBB, BlockIt MBBI, unsigned Opcode,
     return BuildMI(MBB, MBBI, MBBI->getDebugLoc(), TII->get(Opcode), DstReg);
   }
 
   MachineRegisterInfo &getRegInfo(Block &MBB) { return MBB.getParent()->getRegInfo(); }
 
   bool expandArith(unsigned OpLo, unsigned OpHi, Block &MBB, BlockIt MBBI);
   bool expandLogic(unsigned Op, Block &MBB, BlockIt MBBI);
   bool expandLogicImm(unsigned Op, Block &MBB, BlockIt MBBI);
+  bool expandLogicImmNeeded(unsigned Op, unsigned ImmVal);
 
   template<typename Func>
   bool expandAtomic(Block &MBB, BlockIt MBBI, Func f);
 
   template<typename Func>
   bool expandAtomicBinaryOp(unsigned Opcode, Block &MBB, BlockIt MBBI, Func f);
 
   bool expandAtomicBinaryOp(unsigned Opcode, Block &MBB, BlockIt MBBI);
expandLogic(unsigned Op, Block &MBB, BlockIt MBBI) {
   if (ImpIsDead)
     MIBHI->getOperand(3).setIsDead();
 
   MI.eraseFromParent();
   return true;
 }
 
 bool AVRExpandPseudo::
+  expandLogicImmNeeded(unsigned Op, unsigned ImmVal) {
+
+  // ORI Rd, 0x0 is redundant.
+  if (Op == AVR::ORIRdK && ImmVal == 0x0)
+    return false;
+
+  return true;
+}
+
+bool AVRExpandPseudo::
 expandLogicImm(unsigned Op, Block &MBB, BlockIt MBBI) {
   MachineInstr &MI = *MBBI;
   unsigned DstLoReg, DstHiReg;
   unsigned DstReg = MI.getOperand(0).getReg();
   bool DstIsDead = MI.getOperand(0).isDead();
   bool SrcIsKill = MI.getOperand(1).isKill();
   bool ImpIsDead = MI.getOperand(3).isDead();
   unsigned Imm = MI.getOperand(2).getImm();
   unsigned Lo8 = Imm & 0xff;
   unsigned Hi8 = (Imm >> 8) & 0xff;
   TRI->splitReg(DstReg, DstLoReg, DstHiReg);
+  bool Expand;
+
+  Expand = expandLogicImmNeeded(Op, Lo8);
 
+  if (Expand) {
     auto MIBLO = buildMI(MBB, MBBI, Op)
       .addReg(DstLoReg, RegState::Define | getDeadRegState(DstIsDead))
       .addReg(DstLoReg, getKillRegState(SrcIsKill))
       .addImm(Lo8);
 
     // SREG is always implicitly dead
     MIBLO->getOperand(3).setIsDead();
+  }
 
+  Expand = expandLogicImmNeeded(Op, Hi8);
+
+  if (Expand) {
     auto MIBHI = buildMI(MBB, MBBI, Op)
       .addReg(DstHiReg, RegState::Define | getDeadRegState(DstIsDead))
       .addReg(DstHiReg, getKillRegState(SrcIsKill))
       .addImm(Hi8);
 
     if (ImpIsDead)
       MIBHI->getOperand(3).setIsDead();
+  }
 
   MI.eraseFromParent();
   return true;
 }
 
 template <>
 bool AVRExpandPseudo::expand<AVR::ADDWRdRr>(Block &MBB, BlockIt MBBI) {
   return expandArith(AVR::ADDRdRr, AVR::ADCRdRr, MBB, MBBI);

test/CodeGen/AVR/PR31344.ll

+; RUN: llc < %s -march=avr | FileCheck %s
+
+; Unit test for: PR 31344
+
+define i16 @or16_reg_imm_0xff00(i16 %a) {
+; CHECK-LABEL: or16_reg_imm_0xff00
+; CHECK-NOT: ori {{r[0-9]+}}, 0
+; CHECK: ori {{r[0-9]+}}, 255
+    %result = or i16 %a, 65280
+    ret i16 %result
+}
+
+define i16 @or16_reg_imm_0xffb3(i16 %a) {
+; CHECK-LABEL: or16_reg_imm_0xffb3
+; CHECK: ori {{r[0-9]+}}, 179
+; CHECK: ori {{r[0-9]+}}, 255
+    %result = or i16 %a, 65459
+    ret i16 %result
+}
+
+define i16 @or16_reg_imm_0x00ff(i16 %a) {
+; CHECK-LABEL: or16_reg_imm_0x00ff
+; CHECK: ori {{r[0-9]+}}, 255
+; CHECK-NOT: ori {{r[0-9]+}}, 0
+    %result = or i16 %a, 255
+    ret i16 %result
+}
+
+define i16 @or16_reg_imm_0xb3ff(i16 %a) {
+; CHECK-LABEL: or16_reg_imm_0xb3ff
+; CHECK: ori {{r[0-9]+}}, 255
+; CHECK: ori {{r[0-9]+}}, 179
+    %result = or i16 %a, 46079
+    ret i16 %result
+}

test/CodeGen/AVR/integration/blink.ll

 entry:
 
   ; This should load the value of DDRB, OR it with the bit number and store
   ; the result back to DDRB.
 
   ; CHECK:      in      [[TMPREG:r[0-9]+]], 4
   ; CHECK-NEXT: ori     [[TMPREG]], 32
 
-  ; This next line is unnecessary, but we CodeGen it anyway. We should probably optimize this out (PR31344).
-  ; CHECK-NEXT: ori     {{r[0-9]+}}, 0
+  ; CHECK-NOT: ori     {{r[0-9]+}}, 0
 
   ; CHECK-NEXT: out     4, [[TMPREG]]
   ; CHECK-NEXT: ret
 
   %0 = load volatile i8, i8* inttoptr (i16 36 to i8*), align 1
   %conv = zext i8 %0 to i16
   %or = or i16 %conv, 32
   %conv1 = trunc i16 %or to i8
   store volatile i8 %conv1, i8* inttoptr (i16 36 to i8*), align 1
   ret void
 }
 
 ; Turns on the LED.
 ; CHECK-LABEL: turn_on
 define void @turn_on() {
 entry:
 
   ; This should load the value of PORTB, OR it with the bit number and store
   ; the result back to DDRB.
 
   ; CHECK:      in      [[TMPREG:r[0-9]+]], 5
   ; CHECK-NEXT: ori     [[TMPREG]], 32
 
-  ; This next line is unnecessary, but we CodeGen it anyway. We should probably optimize this out (PR31344).
-  ; CHECK-NEXT: ori     {{r[0-9]+}}, 0
+  ; CHECK-NOT: ori     {{r[0-9]+}}, 0
 
   ; CHECK-NEXT: out     5, [[TMPREG]]
   ; CHECK-NEXT: ret
 
   %0 = load volatile i8, i8* inttoptr (i16 37 to i8*), align 1
   %conv = zext i8 %0 to i16
   %or = or i16 %conv, 32
   %conv1 = trunc i16 %or to i8