[ARM] Add some missing thumb1 opcodes to enable peephole optimisation of CMPs

[dmgreen]

Summary

Noticed when codegen prepare started adding more llvm.uadd.with.overflow's, this follows
the same code path as Thumb2, removing redundant CMP's in the peephole optimiser.

[dmgreen]

lib/Target/ARM/ARMBaseInstrInfo.cpp

2936–2937

Whoops. Forgot about this bit.

[efriedma]

lib/Target/ARM/ARMBaseInstrInfo.cpp

2625

Can you commit the reformatting separately? It's a little confusing to distinguish which changes are actually relevant.

2678

What are these "isReg" checks supposed to do? You already checked the opcode.

[dmgreen]

Fixed that bit I missed, plus better formatting and cleanup

[efriedma]

I'd like to see testcases for all the possible add/sub opcodes, if it isn't too much work.

lib/Target/ARM/ARMBaseInstrInfo.cpp

2678

The corresponding ARM/Thumb2 code checks for ADDrr; should we add the corresponding check for tADDrr?
Or does that not work for some reason?

2693

I guess it's sort of orthogonal to this patch, but this list isn't complete, if it's supposed to be a list of instructions which update the NZ bits. (Missing tADC, tSBC, tAND, tORR, tEOR, tBIC, tMVN, tASRri, tASRrr, tROR, I think.)

[dmgreen]

lib/Target/ARM/ARMBaseInstrInfo.cpp

2678

Thanks, that was lost in a reshuffle I think. It does show up some machine verifier problems with this. IsThumb1 isn't being set correctly

[dmgreen]

Updates for IsThumb1

[efriedma]

LGTM

lib/Target/ARM/ARMBaseInstrInfo.cpp

bool ARMBaseInstrInfo::analyzeCompare(const MachineInstr &MI, unsigned &SrcReg,
   case ARM::tCMPi8:
     SrcReg = MI.getOperand(0).getReg();
     SrcReg2 = 0;
     CmpMask = ~0;
     CmpValue = MI.getOperand(1).getImm();
     return true;
   case ARM::CMPrr:
   case ARM::t2CMPrr:
+  case ARM::tCMPr:
     SrcReg = MI.getOperand(0).getReg();
     SrcReg2 = MI.getOperand(1).getReg();
     CmpMask = ~0;
     CmpValue = 0;
     return true;
   case ARM::TSTri:
   case ARM::t2TSTri:
     SrcReg = MI.getOperand(0).getReg();

 /// isRedundantFlagInstr - check whether the first instruction, whose only
 /// purpose is to update flags, can be made redundant.
 /// CMPrr can be made redundant by SUBrr if the operands are the same.
 /// CMPri can be made redundant by SUBri if the operands are the same.
 /// CMPrr(r0, r1) can be made redundant by ADDr[ri](r0, r1, X).
 /// This function can be extended later on.
 inline static bool isRedundantFlagInstr(const MachineInstr *CmpI,
                                         unsigned SrcReg, unsigned SrcReg2,
-                                        int ImmValue, const MachineInstr *OI) {
+                                        int ImmValue, const MachineInstr *OI,
+                                        bool &IsThumb1) {
   if ((CmpI->getOpcode() == ARM::CMPrr || CmpI->getOpcode() == ARM::t2CMPrr) &&
       (OI->getOpcode() == ARM::SUBrr || OI->getOpcode() == ARM::t2SUBrr) &&
       ((OI->getOperand(1).getReg() == SrcReg &&
         OI->getOperand(2).getReg() == SrcReg2) ||
        (OI->getOperand(1).getReg() == SrcReg2 &&
-        OI->getOperand(2).getReg() == SrcReg)))
+        OI->getOperand(2).getReg() == SrcReg))) {
+    IsThumb1 = false;
+    return true;
+  }
+
+  if (CmpI->getOpcode() == ARM::tCMPr && OI->getOpcode() == ARM::tSUBrr &&
+      ((OI->getOperand(2).getReg() == SrcReg &&
+        OI->getOperand(3).getReg() == SrcReg2) ||
+       (OI->getOperand(2).getReg() == SrcReg2 &&
+        OI->getOperand(3).getReg() == SrcReg))) {
+    IsThumb1 = true;
     return true;
+  }
 
   if ((CmpI->getOpcode() == ARM::CMPri || CmpI->getOpcode() == ARM::t2CMPri) &&
       (OI->getOpcode() == ARM::SUBri || OI->getOpcode() == ARM::t2SUBri) &&
       OI->getOperand(1).getReg() == SrcReg &&
-      OI->getOperand(2).getImm() == ImmValue)
+      OI->getOperand(2).getImm() == ImmValue) {
+    IsThumb1 = false;
     return true;
+  }
+
+  if (CmpI->getOpcode() == ARM::tCMPi8 &&
+      (OI->getOpcode() == ARM::tSUBi8 || OI->getOpcode() == ARM::tSUBi3) &&
+      OI->getOperand(2).getReg() == SrcReg &&
+      OI->getOperand(3).getImm() == ImmValue) {
+    IsThumb1 = true;
+    return true;
+  }
 
   if ((CmpI->getOpcode() == ARM::CMPrr || CmpI->getOpcode() == ARM::t2CMPrr) &&
       (OI->getOpcode() == ARM::ADDrr || OI->getOpcode() == ARM::t2ADDrr ||
        OI->getOpcode() == ARM::ADDri || OI->getOpcode() == ARM::t2ADDri) &&
-      OI->getOperand(0).isReg() && OI->getOperand(1).isReg() &&
       OI->getOperand(0).getReg() == SrcReg &&
-      OI->getOperand(1).getReg() == SrcReg2)
+      OI->getOperand(1).getReg() == SrcReg2) {
+    IsThumb1 = false;
+    return true;
+  }
+
+  if (CmpI->getOpcode() == ARM::tCMPr &&
+      (OI->getOpcode() == ARM::tADDi3 || OI->getOpcode() == ARM::tADDi8 ||
+       OI->getOpcode() == ARM::tADDrr) &&
+      OI->getOperand(0).getReg() == SrcReg &&
+      OI->getOperand(2).getReg() == SrcReg2) {
+    IsThumb1 = true;
     return true;
+  }
+
   return false;
 }
 
 static bool isOptimizeCompareCandidate(MachineInstr *MI, bool &IsThumb1) {
   switch (MI->getOpcode()) {
   default: return false;
   case ARM::tLSLri:
   case ARM::tLSRri:
   case ARM::tLSLrr:
   case ARM::tLSRrr:
   case ARM::tSUBrr:
   case ARM::tADDrr:
   case ARM::tADDi3:
   case ARM::tADDi8:
   case ARM::tSUBi3:
   case ARM::tSUBi8:
   case ARM::tMUL:
     IsThumb1 = true;
     LLVM_FALLTHROUGH;
   case ARM::RSBrr:
   case ARM::RSBri:
   case ARM::RSCrr:
   case ARM::RSCri:
   case ARM::ADDrr:
   case ARM::ADDri:
if (SrcReg2 != 0)
     // MI is not a candidate for CMPrr.
     MI = nullptr;
   else if (MI->getParent() != CmpInstr.getParent() || CmpValue != 0) {
     // Conservatively refuse to convert an instruction which isn't in the same
     // BB as the comparison.
     // For CMPri w/ CmpValue != 0, a SubAdd may still be a candidate.
     // Thus we cannot return here.
     if (CmpInstr.getOpcode() == ARM::CMPri ||
-        CmpInstr.getOpcode() == ARM::t2CMPri)
+        CmpInstr.getOpcode() == ARM::t2CMPri ||
+        CmpInstr.getOpcode() == ARM::tCMPi8)
       MI = nullptr;
     else
       return false;
   }
 
   bool IsThumb1 = false;
   if (MI && !isOptimizeCompareCandidate(MI, IsThumb1))
     return false;
if (MI && IsThumb1) {
     }
     I = CmpInstr;
     E = MI;
   }
 
   // Check that CPSR isn't set between the comparison instruction and the one we
   // want to change. At the same time, search for SubAdd.
   const TargetRegisterInfo *TRI = &getRegisterInfo();
+  bool SubAddIsThumb1 = false;
   do {
     const MachineInstr &Instr = *--I;
 
     // Check whether CmpInstr can be made redundant by the current instruction.
-    if (isRedundantFlagInstr(&CmpInstr, SrcReg, SrcReg2, CmpValue, &Instr)) {
+    if (isRedundantFlagInstr(&CmpInstr, SrcReg, SrcReg2, CmpValue, &Instr,
+                             SubAddIsThumb1)) {
       SubAdd = &*I;
       break;
     }
 
     // Allow E (which was initially MI) to be SubAdd but do not search before E.
     if (I == E)
       break;
 
     if (Instr.modifiesRegister(ARM::CPSR, TRI) ||
         Instr.readsRegister(ARM::CPSR, TRI))
       // This instruction modifies or uses CPSR after the one we want to
       // change. We can't do this transformation.
       return false;
 
   } while (I != B);
 
   // Return false if no candidates exist.
   if (!MI && !SubAdd)
     return false;
 
   // If we found a SubAdd, use it as it will be "closer"
   if (SubAdd) {
     MI = SubAdd;
-    IsThumb1 = false;
+    IsThumb1 = SubAddIsThumb1;
   }
 
   // We can't use a predicated instruction - it doesn't always write the flags.
   if (isPredicated(*MI))
     return false;
 
   // Scan forward for the use of CPSR
   // When checking against MI: if it's a conditional code that requires
for (unsigned IO = 0, EO = Instr.getNumOperands();
         // If we have SUB(r1, r2) and CMP(r2, r1), the condition code based
         // on CMP needs to be updated to be based on SUB.
         // If we have ADD(r1, r2, X) and CMP(r1, r2), the condition code also
         // needs to be modified.
         // Push the condition code operands to OperandsToUpdate.
         // If it is safe to remove CmpInstr, the condition code of these
         // operands will be modified.
         unsigned Opc = SubAdd->getOpcode();
         bool IsSub = Opc == ARM::SUBrr || Opc == ARM::t2SUBrr ||
-                     Opc == ARM::SUBri || Opc == ARM::t2SUBri;
-        if (!IsSub || (SrcReg2 != 0 && SubAdd->getOperand(1).getReg() == SrcReg2 &&
-                       SubAdd->getOperand(2).getReg() == SrcReg)) {
+                     Opc == ARM::SUBri || Opc == ARM::t2SUBri ||
+                     Opc == ARM::tSUBrr || Opc == ARM::tSUBi3 ||
+                     Opc == ARM::tSUBi8;
+        unsigned OpI = Opc != ARM::tSUBrr ? 1 : 2;
+        if (!IsSub ||
+            (SrcReg2 != 0 && SubAdd->getOperand(OpI).getReg() == SrcReg2 &&
+             SubAdd->getOperand(OpI + 1).getReg() == SrcReg)) {
           // VSel doesn't support condition code update.
           if (IsInstrVSel)
             return false;
           // Ensure we can swap the condition.
           ARMCC::CondCodes NewCC = (IsSub ? getSwappedCondition(CC) : getCmpToAddCondition(CC));
           if (NewCC == ARMCC::AL)
             return false;
           OperandsToUpdate.push_back(
bool ARMBaseInstrInfo::shouldSink(const MachineInstr &MI) const {
   // We heuristically only look at the instruction immediately following MI to
   // avoid potentially searching the entire basic block.
   if (isPredicated(MI))
     return true;
   MachineBasicBlock::const_iterator Next = &MI;
   ++Next;
   unsigned SrcReg, SrcReg2;
   int CmpMask, CmpValue;
+  bool IsThumb1;
   if (Next != MI.getParent()->end() &&
       analyzeCompare(*Next, SrcReg, SrcReg2, CmpMask, CmpValue) &&
-      isRedundantFlagInstr(&*Next, SrcReg, SrcReg2, CmpValue, &MI))
+      isRedundantFlagInstr(&*Next, SrcReg, SrcReg2, CmpValue, &MI, IsThumb1))
     return false;
   return true;
 }
 
 bool ARMBaseInstrInfo::FoldImmediate(MachineInstr &UseMI, MachineInstr &DefMI,
                                      unsigned Reg,
                                      MachineRegisterInfo *MRI) const {
   // Fold large immediates into add, sub, or, xor.

test/CodeGen/ARM/intrinsics-overflow.ll

define i32 @sadd_overflow(i32 %a, i32 %b) #0 {
 
   ; CHECK-LABEL: sadd_overflow:
 
   ; ARM: adds r[[R2:[0-9]+]], r[[R0:[0-9]+]], r[[R1:[0-9]+]]
   ; ARM: mov r[[R0]], #1
   ; ARM: movvc r[[R0]], #0
   ; ARM: mov pc, lr
 
-  ; THUMBV6: adds    r1, r0, r1
-  ; THUMBV6: cmp     r1, r0
+  ; THUMBV6: adds    r0, r0, r1
   ; THUMBV6: bvc     .LBB1_2
 
   ; THUMBV7: adds  r[[R2:[0-9]+]], r[[R0]], r[[R1:[0-9]+]]
   ; THUMBV7: mov.w r[[R0:[0-9]+]], #1
   ; THUMBV7: it    vc
   ; THUMBV7: movvc r[[R0]], #0
 }
 

test/CodeGen/Thumb/peephole-cmp.mir

bb.2:
     %3:tgpr, dead $cpsr = tMOVi8 0, 14, $noreg
     $r0 = COPY %3
     tBX_RET 14, $noreg, implicit $r0
 
 # CHECK-LABEL: name: test_subrr
 # CHECK:      [[COPY1:%[0-9]+]]:tgpr = COPY $r1
 # CHECK-NEXT: [[COPY0:%[0-9]+]]:tgpr = COPY $r0
 # CHECK-NEXT: [[ADD:%[0-9]+]]:tgpr, $cpsr = tSUBrr [[COPY1]], [[COPY0]], 14, $noreg
-# CHECK-NEXT: tCMPr [[COPY0]], [[COPY1]], 14, $noreg, implicit-def $cpsr
-# CHECK-NEXT: tBcc %bb.2, 3, $cpsr
+# CHECK-NEXT: tBcc %bb.2, 8, $cpsr
 ...
 ---
 name:            test_subrr_c
 liveins:
   - { reg: '$r0', virtual-reg: '%1' }
   - { reg: '$r1', virtual-reg: '%2' }
 body:             |
   bb.0:
bb.2:
     %3:tgpr, dead $cpsr = tMOVi8 0, 14, $noreg
     $r0 = COPY %3
     tBX_RET 14, $noreg, implicit $r0
 
 # CHECK-LABEL: name: test_subrr_c
 # CHECK:      [[COPY1:%[0-9]+]]:tgpr = COPY $r1
 # CHECK-NEXT: [[COPY0:%[0-9]+]]:tgpr = COPY $r0
 # CHECK-NEXT: [[ADD:%[0-9]+]]:tgpr, $cpsr = tSUBrr [[COPY0]], [[COPY1]], 14, $noreg
-# CHECK-NEXT: tCMPr [[COPY0]], [[COPY1]], 14, $noreg, implicit-def $cpsr
 # CHECK-NEXT: tBcc %bb.2, 3, $cpsr
 ...
 ---
 name:            test_subri3
 liveins:
   - { reg: '$r0', virtual-reg: '%1' }
 body:             |
   bb.0:
body:             |
   bb.2:
     %2:tgpr, dead $cpsr = tMOVi8 0, 14, $noreg
     $r0 = COPY %2
     tBX_RET 14, $noreg, implicit $r0
 
 # CHECK-LABEL: name: test_subri3
 # CHECK:      [[COPY:%[0-9]+]]:tgpr = COPY $r0
 # CHECK-NEXT: [[ADD:%[0-9]+]]:tgpr, $cpsr = tSUBi3 [[COPY]], 1, 14, $noreg
-# CHECK-NEXT: tCMPi8 [[COPY]], 1, 14, $noreg, implicit-def $cpsr
 # CHECK-NEXT: tBcc %bb.2, 3, $cpsr
 ...
 ---
 name:            test_subri8
 liveins:
   - { reg: '$r0', virtual-reg: '%1' }
 body:             |
   bb.0:
body:             |
   bb.2:
     %2:tgpr, dead $cpsr = tMOVi8 0, 14, $noreg
     $r0 = COPY %2
     tBX_RET 14, $noreg, implicit $r0
 
 # CHECK-LABEL: name: test_subri8
 # CHECK:      [[COPY:%[0-9]+]]:tgpr = COPY $r0
 # CHECK-NEXT: [[ADD:%[0-9]+]]:tgpr, $cpsr = tSUBi8 [[COPY]], 1, 14, $noreg
-# CHECK-NEXT: tCMPi8 [[COPY]], 1, 14, $noreg, implicit-def $cpsr
 # CHECK-NEXT: tBcc %bb.2, 3, $cpsr
 ...
 ---
 name:            test_addrr
 liveins:
   - { reg: '$r0', virtual-reg: '%1' }
   - { reg: '$r1', virtual-reg: '%2' }
 body:             |
bb.2:
     %3:tgpr, dead $cpsr = tMOVi8 0, 14, $noreg
     $r0 = COPY %3
     tBX_RET 14, $noreg, implicit $r0
 
 # CHECK-LABEL: name: test_addrr
 # CHECK:      [[COPY1:%[0-9]+]]:tgpr = COPY $r1
 # CHECK-NEXT: [[COPY0:%[0-9]+]]:tgpr = COPY $r0
 # CHECK-NEXT: [[ADD:%[0-9]+]]:tgpr, $cpsr = tADDrr [[COPY1]], [[COPY0]], 14, $noreg
-# CHECK-NEXT: tCMPr [[ADD]], [[COPY1]], 14, $noreg, implicit-def $cpsr
-# CHECK-NEXT: tBcc %bb.2, 3, $cpsr
+# CHECK-NEXT: tBcc %bb.2, 2, $cpsr
 ...
 ---
 name:            test_addri3
 liveins:
   - { reg: '$r0', virtual-reg: '%1' }
 body:             |
   bb.0:
     successors: %bb.2(0x40000000), %bb.1(0x40000000)
body:             |
   bb.2:
     %2:tgpr, dead $cpsr = tMOVi8 0, 14, $noreg
     $r0 = COPY %2
     tBX_RET 14, $noreg, implicit $r0
 
 # CHECK-LABEL: name: test_addri3
 # CHECK:      [[COPY:%[0-9]+]]:tgpr = COPY $r0
 # CHECK-NEXT: [[ADD:%[0-9]+]]:tgpr, $cpsr = tADDi3 [[COPY]], 1, 14, $noreg
-# CHECK-NEXT: tCMPr [[ADD]], [[COPY]], 14, $noreg, implicit-def $cpsr
-# CHECK-NEXT: tBcc %bb.2, 3, $cpsr
+# CHECK-NEXT: tBcc %bb.2, 2, $cpsr
 ...
 ---
 name:            test_addri8
 liveins:
   - { reg: '$r0', virtual-reg: '%1' }
 body:             |
   bb.0:
     successors: %bb.2(0x40000000), %bb.1(0x40000000)
body:             |
   bb.2:
     %2:tgpr, dead $cpsr = tMOVi8 0, 14, $noreg
     $r0 = COPY %2
     tBX_RET 14, $noreg, implicit $r0
 
 # CHECK-LABEL: name: test_addri8
 # CHECK:      [[COPY:%[0-9]+]]:tgpr = COPY $r0
 # CHECK-NEXT: [[ADD:%[0-9]+]]:tgpr, $cpsr = tADDi8 [[COPY]], 10, 14, $noreg
-# CHECK-NEXT: tCMPr [[ADD]], [[COPY]], 14, $noreg, implicit-def $cpsr
-# CHECK-NEXT: tBcc %bb.2, 3, $cpsr
+# CHECK-NEXT: tBcc %bb.2, 2, $cpsr
 ...