Fix ARM peephole optimizeCompare to avoid optimizing unsigned cmp to 0.

[jvoung]

Summary

Previously it only avoided optimizing signed comparisons to 0.
Sometimes the DAGCombiner will optimize the unsigned comparisons
to 0 before it gets to the peephole pass, but sometimes it doesn't.

Fix for PR22373.

[jfb]

lgtm otherwise.

lib/Target/ARM/ARMBaseInstrInfo.cpp

2574

I think it would be better to rewire the code as:

switch (CC) {
case ARMCC::EQ: // Z
case ARMCC::NE: // Z
case ARMCC::MI: // N
case ARMCC::PL: // N
case ARMCC::AL: // none
  // CPSR can be used multiple times, we should continue.
  break;
case ARMCC::CS: // C
case ARMCC::CC: // C
case ARMCC::VS: // V
case ARMCC::VC: // V
case ARMCC::HI: // C Z
case ARMCC::LS: // C Z
case ARMCC::GE: // N V
case ARMCC::LT: // N V
case ARMCC::GT: // Z N V
case ARMCC::LE: // Z N V
  // The instruction uses the V bit or C bit which is not safe.
  return false;
}

[jvoung]

Apply JF's review suggestion -- annotate the switch cases w/ the bits used.

lib/Target/ARM/ARMBaseInstrInfo.cpp

optimizeCompareInstr(MachineInstr *CmpInstr, unsigned SrcReg, unsigned SrcReg2,
   // For CMPri(r1, CmpValue), we are looking for SUBri(r1, CmpValue).
   MachineInstr *Sub = nullptr;
   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, we need to check Sub, thus we can't return here.
+    // For CMPri w/ CmpValue != 0, a Sub may still be a candidate.
+    // Thus we cannot return here.
     if (CmpInstr->getOpcode() == ARM::CMPri ||
        CmpInstr->getOpcode() == ARM::t2CMPri)
       MI = nullptr;
     else
       return false;
   }
 
   // Check that CPSR isn't set between the comparison instruction and the one we
optimizeCompareInstr(MachineInstr *CmpInstr, unsigned SrcReg, unsigned SrcReg2,
   case ARM::ORRri:
   case ARM::t2ORRrr:
   case ARM::t2ORRri:
   case ARM::EORrr:
   case ARM::EORri:
   case ARM::t2EORrr:
   case ARM::t2EORri: {
     // Scan forward for the use of CPSR
-    // When checking against MI: if it's a conditional code requires
-    // checking of V bit, then this is not safe to do.
+    // When checking against MI: if it's a conditional code that requires
+    // checking of the V bit or C bit, then this is not safe to do.
     // It is safe to remove CmpInstr if CPSR is redefined or killed.
     // If we are done with the basic block, we need to check whether CPSR is
     // live-out.
     SmallVector<std::pair<MachineOperand*, ARMCC::CondCodes>, 4>
         OperandsToUpdate;
     bool isSafe = false;
     I = CmpInstr;
     E = CmpInstr->getParent()->end();
while (!isSafe && ++I != E) {
           if (SrcReg2 != 0 && Sub->getOperand(1).getReg() == SrcReg2 &&
               Sub->getOperand(2).getReg() == SrcReg) {
             // VSel doesn't support condition code update.
             if (IsInstrVSel)
               return false;
             OperandsToUpdate.push_back(
                 std::make_pair(&((*I).getOperand(IO - 1)), NewCC));
           }
-        } else
+        } else {
+          // No Sub, so this is x = <op> y, z; cmp x, 0.
           switch (CC) {
-          default:
+          case ARMCC::EQ: // Z
+          case ARMCC::NE: // Z
+          case ARMCC::MI: // N
+          case ARMCC::PL: // N
+          case ARMCC::AL: // none
             // CPSR can be used multiple times, we should continue.
             break;
-          case ARMCC::VS:
-          case ARMCC::VC:
-          case ARMCC::GE:
-          case ARMCC::LT:
-          case ARMCC::GT:
-          case ARMCC::LE:
+          case ARMCC::HS: // C
+          case ARMCC::LO: // C
+          case ARMCC::VS: // V
+          case ARMCC::VC: // V
+          case ARMCC::HI: // C Z
+          case ARMCC::LS: // C Z
+          case ARMCC::GE: // N V
+          case ARMCC::LT: // N V
+          case ARMCC::GT: // Z N V
+          case ARMCC::LE: // Z N V
+            // The instruction uses the V bit or C bit which is not safe.
             return false;
           }
         }
       }
+    }
 
     // If CPSR is not killed nor re-defined, we should check whether it is
     // live-out. If it is live-out, do not optimize.
     if (!isSafe) {
       MachineBasicBlock *MBB = CmpInstr->getParent();
       for (MachineBasicBlock::succ_iterator SI = MBB->succ_begin(),
                SE = MBB->succ_end(); SI != SE; ++SI)
         if ((*SI)->isLiveIn(ARM::CPSR))

test/CodeGen/ARM/sub-cmp-peephole.ll

 
 land.lhs.true:                                    ; preds = %num2long.exit
   ret i32 17
 
 if.end11:                                         ; preds = %num2long.exit
   ret i32 23
 }
 
+; When considering the producer of cmp's src as the subsuming instruction,
+; only consider that when the comparison is to 0.
+define i32 @cmp_src_nonzero(i32 %a, i32 %b, i32 %x, i32 %y) {
+entry:
+; CHECK-LABEL: cmp_src_nonzero:
+; CHECK: sub
+; CHECK: cmp
+  %sub = sub i32 %a, %b
+  %cmp = icmp eq i32 %sub, 17
+  %ret = select i1 %cmp, i32 %x, i32 %y
+  ret i32 %ret
+}
+
 define float @float_sel(i32 %a, i32 %b, float %x, float %y) {
 entry:
 ; CHECK-LABEL: float_sel:
 ; CHECK-NOT: cmp
 ; V8-LABEL: float_sel:
 ; V8-NOT: cmp
 ; V8: vseleq.f32
   %sub = sub i32 %a, %b

 ; V8: cmp
 ; V8: vsel
   %cmp = icmp sgt i32 %a, %b
   %sub = sub i32 %b, %a
   %ret = select i1 %cmp, double %x, double %y
   store i32 %sub, i32* @t
   ret double %ret
 }
+
+declare void @abort()
+declare void @exit(i32)
+
+; If the comparison uses the V bit (signed overflow/underflow), we can't
+; omit the comparison.
+define i32 @cmp_slt0(i32 %a, i32 %b, i32 %x, i32 %y) {
+entry:
+; CHECK-LABEL: cmp_slt0
+; CHECK: sub
+; CHECK: cmp
+; CHECK: bge
+  %load = load i32* @t, align 4
+  %sub = sub i32 %load, 17
+  %cmp = icmp slt i32 %sub, 0
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:
+  call void @abort()
+  unreachable
+
+if.else:
+  call void @exit(i32 0)
+  unreachable
+}
+
+; Same for the C bit. (Note the ult X, 0 is trivially
+; false, so the DAG combiner may or may not optimize it).
+define i32 @cmp_ult0(i32 %a, i32 %b, i32 %x, i32 %y) {
+entry:
+; CHECK-LABEL: cmp_ult0
+; CHECK: sub
+; CHECK: cmp
+; CHECK: bhs
+  %load = load i32* @t, align 4
+  %sub = sub i32 %load, 17
+  %cmp = icmp ult i32 %sub, 0
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:
+  call void @abort()
+  unreachable
+
+if.else:
+  call void @exit(i32 0)
+  unreachable
+}