[ARM] Treat cmn immediates as legal in isLegalICmpImmediate.

[efriedma]

Summary

The original code attempted to do this, but the std::abs() call didn't actually do anything due to implicit type conversions. Fix the type conversions, and perform the correct check for negated immediates.

This probably has very little practical impact, but it's worth fixing just to avoid confusion in the future, I think.

[samparker]

lib/Target/ARM/ARMISelLowering.cpp

3861 ↗

(On Diff #154023)

I don't understand what this is trying to do, why the special case? Surely the fix in isLegalICmpImmediate is enough?

[efriedma]

lib/Target/ARM/ARMISelLowering.cpp

3861 ↗

(On Diff #154023)

I probably should have added a comment to this.

The special case for "-1" isn't strictly necessary; it just prefers "cmp r0, #0" over "cmn r0, #1" like the old code does, to reduce the number of regression test changes. Maybe that doesn't matter?

[samparker]

lib/Target/ARM/ARMISelLowering.cpp

3861 ↗

(On Diff #154023)

It really confused me, so I feel that changing the tests is a necessary chore to make the code easier to understand.

[efriedma]

Remove special-case for -1.

[samparker]

LGTM, thanks.

llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp

 
 /// Returns appropriate ARM CMP (cmp) and corresponding condition code for
 /// the given operands.
 SDValue ARMTargetLowering::getARMCmp(SDValue LHS, SDValue RHS, ISD::CondCode CC,
                                      SDValue &ARMcc, SelectionDAG &DAG,
                                      const SDLoc &dl) const {
   if (ConstantSDNode *RHSC = dyn_cast<ConstantSDNode>(RHS.getNode())) {
     unsigned C = RHSC->getZExtValue();
-    if (!isLegalICmpImmediate(C)) {
-      // Constant does not fit, try adjusting it by one?
+    if (!isLegalICmpImmediate((int32_t)C)) {
+      // Constant does not fit, try adjusting it by one.
       switch (CC) {
       default: break;
       case ISD::SETLT:
       case ISD::SETGE:
         if (C != 0x80000000 && isLegalICmpImmediate(C-1)) {
           CC = (CC == ISD::SETLT) ? ISD::SETLE : ISD::SETGT;
           RHS = DAG.getConstant(C - 1, dl, MVT::i32);
         }

 
 /// isLegalICmpImmediate - Return true if the specified immediate is legal
 /// icmp immediate, that is the target has icmp instructions which can compare
 /// a register against the immediate without having to materialize the
 /// immediate into a register.
 bool ARMTargetLowering::isLegalICmpImmediate(int64_t Imm) const {
   // Thumb2 and ARM modes can use cmn for negative immediates.
   if (!Subtarget->isThumb())
-    return ARM_AM::getSOImmVal(std::abs(Imm)) != -1;
+    return ARM_AM::getSOImmVal((uint32_t)Imm) != -1 ||
+           ARM_AM::getSOImmVal(-(uint32_t)Imm) != -1;
   if (Subtarget->isThumb2())
-    return ARM_AM::getT2SOImmVal(std::abs(Imm)) != -1;
+    return ARM_AM::getT2SOImmVal((uint32_t)Imm) != -1 ||
+           ARM_AM::getT2SOImmVal(-(uint32_t)Imm) != -1;
   // Thumb1 doesn't have cmn, and only 8-bit immediates.
   return Imm >= 0 && Imm <= 255;
 }
 
 /// isLegalAddImmediate - Return true if the specified immediate is a legal add
 /// *or sub* immediate, that is the target has add or sub instructions which can
 /// add a register with the immediate without having to materialize the
 /// immediate into a register.

llvm/trunk/test/CodeGen/ARM/arm-shrink-wrapping-linux.ll

 ; restore sequence. I.e., we restored too early.
 ;
 ; CHECK-LABEL: wrongUseOfPostDominate:
 ;
 ; The prologue is the first thing happening in the function
 ; without shrink-wrapping.
 ; DISABLE: push
 ;
-; CHECK: cmp r1, #0
+; CHECK: cmn r1, #1
 ;
 ; With shrink-wrapping, we branch to a pre-header, where the prologue
 ; is located.
-; ENABLE-NEXT: blt [[LOOP_PREHEADER:[.a-zA-Z0-9_]+]]
+; ENABLE-NEXT: ble [[LOOP_PREHEADER:[.a-zA-Z0-9_]+]]
 ; Without shrink-wrapping, we go straight into the loop.
-; DISABLE-NEXT: blt [[LOOP_HEADER:[.a-zA-Z0-9_]+]]
+; DISABLE-NEXT: ble [[LOOP_HEADER:[.a-zA-Z0-9_]+]]
 ;
 ; CHECK: @ %if.end29
 ; DISABLE-NEXT: pop
 ; ENABLE-NEXT: bx lr
 ;
 ; ENABLE: [[LOOP_PREHEADER]]
 ; ENABLE: push
 ; We must not find a pop here, otherwise that means we are in the loop

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

 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
+; CHECK: cmn
+; CHECK: bgt
   %load = load i32, 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

llvm/trunk/test/CodeGen/Thumb2/thumb2-cmp.ll

true:
   ret i32 1
 
 false:
   ret i32 2
 }
 
 define i32 @slt_neg_soimm(i32 %a) {
 ; CHECK-LABEL: slt_neg_soimm:
-; CHECK: mvn     r1, #7929856
+; CHECK: cmn.w r0, #7929856
   %b = icmp slt i32 %a, -7929856
   br i1 %b, label %true, label %false
 
 true:
   ret i32 1
 
 false:
   ret i32 2
true:
   ret i32 1
 
 false:
   ret i32 2
 }
 
 define i32 @sgt_neg_soimm(i32 %a) {
 ; CHECK-LABEL: sgt_neg_soimm:
-; CHECK: movs    r1, #1
-; CHECK: movt    r1, #65415
+; CHECK: cmn.w r0, #7929856
   %b = icmp sgt i32 %a, -7929856
   br i1 %b, label %true, label %false
 
 true:
   ret i32 1
 
 false:
   ret i32 2