[ARM] don't transform an add(ext Cond), C to select unless there's a setcc of the condition

[spatel]

Summary

The transform in question claims to be doing:

// fold (add (select cc, 0, c), x) -> (select cc, x, (add, x, c))

...starting in PerformADDCombineWithOperands(), but it wasn't actually checking for a setcc node for the sext/zext patterns.

This is exactly the opposite of a transform I'd like to add to DAGCombiner's foldSelectOfConstants(), so I was seeing infinite loops with my draft of a patch applied.

Selecting condition-flag-setting instructions (ands/adds) in "test_tst_assessment" may be a separate bug?

[efriedma]

Selecting condition-flag-setting instructions (ands/adds) in "test_tst_assessment" may be a separate bug?

That testcase definitely isn't testing what it's supposed to test anymore. Can you change it to generate approximately the same code as before? (Basically something like "a &= 3; return b & 3 ? a : a-1;".)

The change essentially looks fine; the point of the transform is to favor a predicated operation over a predicated mov, but that only makes sense if we were actually going to generate a predicated move in the first place.

lib/Target/ARM/ARMISelLowering.cpp

9166 ↗

(On Diff #89721)

How can "!CC.getNode()" possibly be true? CC was just set on the previous line.

[spatel]

Patch updated based on Eli's feedback (thanks!):

1. Don't check CC.getNode() because that's obviously unnecessary.
2. Change "test_tst_assessment" to preserve the intent of that test (now there's only a diff for a thumb target).

[spatel]

In D30355#687423, @spatel wrote:

Patch updated based on Eli's feedback (thanks!):

1. Don't check CC.getNode() because that's obviously unnecessary.
2. Change "test_tst_assessment" to preserve the intent of that test (now there's only a diff for a thumb target).

Actually, there's no diff on that test from the proposed code change in this patch, so I should commit that before this patch as an independent change?

[efriedma]

LGTM.

Actually, there's no diff on that test from the proposed code change in this patch, so I should commit that before this patch as an independent change?

If you want; doesn't really matter either way.

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

case ISD::ZERO_EXTEND:
     // (zext cc) can never be the all ones value.
     if (AllOnes)
       return false;
     LLVM_FALLTHROUGH;
   case ISD::SIGN_EXTEND: {
     SDLoc dl(N);
     EVT VT = N->getValueType(0);
     CC = N->getOperand(0);
-    if (CC.getValueType() != MVT::i1)
+    if (CC.getValueType() != MVT::i1 || CC.getOpcode() != ISD::SETCC)
       return false;
     Invert = !AllOnes;
     if (AllOnes)
       // When looking for an AllOnes constant, N is an sext, and the 'other'
       // value is 0.
       OtherOp = DAG.getConstant(0, dl, VT);
     else if (N->getOpcode() == ISD::ZERO_EXTEND)
       // When looking for a 0 constant, N can be zext or sext.

llvm/trunk/test/CodeGen/ARM/arm-and-tst-peephole.ll

 }
 
 
 ; We were looking through multiple COPY instructions to find an AND we might
 ; fold into a TST, but in doing so we changed the register being tested allowing
 ; folding of unrelated tests (in this case, a TST against r1 was eliminated in
 ; favour of an AND of r0).
 
-define i32 @test_tst_assessment(i1 %lhs, i1 %rhs) {
+define i32 @test_tst_assessment(i32 %a, i32 %b) {
 ; ARM-LABEL: test_tst_assessment:
 ; ARM:       @ BB#0:
 ; ARM-NEXT:    and r0, r0, #1
 ; ARM-NEXT:    tst r1, #1
 ; ARM-NEXT:    subne r0, r0, #1
 ; ARM-NEXT:    mov pc, lr
 ;
 ; THUMB-LABEL: test_tst_assessment:
 ; THUMB:       @ BB#0:
-; THUMB-NEXT:    movs r2, #1
-; THUMB-NEXT:    ands r2, r0
-; THUMB-NEXT:    subs r0, r2, #1
+; THUMB-NEXT:    push {r0}
+; THUMB-NEXT:    pop {r2}
+; THUMB-NEXT:    movs r0, #1
+; THUMB-NEXT:    ands r0, r2
+; THUMB-NEXT:    subs r2, r0, #1
 ; THUMB-NEXT:    lsls r1, r1, #31
-; THUMB-NEXT:    bne .LBB2_2
+; THUMB-NEXT:    beq .LBB2_2
 ; THUMB-NEXT:  @ BB#1:
 ; THUMB-NEXT:    push {r2}
 ; THUMB-NEXT:    pop {r0}
 ; THUMB-NEXT:  .LBB2_2:
 ; THUMB-NEXT:    bx lr
 ;
 ; T2-LABEL: test_tst_assessment:
 ; T2:       @ BB#0:
 ; T2-NEXT:    lsls r1, r1, #31
 ; T2-NEXT:    and r0, r0, #1
 ; T2-NEXT:    it ne
 ; T2-NEXT:    subne r0, #1
 ; T2-NEXT:    bx lr
 ;
 ; V8-LABEL: test_tst_assessment:
 ; V8:       @ BB#0:
 ; V8-NEXT:    lsls r1, r1, #31
 ; V8-NEXT:    and r0, r0, #1
 ; V8-NEXT:    it ne
 ; V8-NEXT:    subne r0, #1
 ; V8-NEXT:    bx lr
-  %lhs32 = zext i1 %lhs to i32
-  %rhs32 = zext i1 %rhs to i32
-  %diff = sub nsw i32 %lhs32, %rhs32
-  ret i32 %diff
+  %and1 = and i32 %a, 1
+  %sub = sub i32 %and1, 1
+  %and2 = and i32 %b, 1
+  %cmp = icmp eq i32 %and2, 0
+  %sel = select i1 %cmp, i32 %and1, i32 %sub
+  ret i32 %sel
 }
 
 !1 = !{!"branch_weights", i32 1, i32 1, i32 3, i32 2 }

llvm/trunk/test/CodeGen/ARM/select_const.ll

 ; CHECK-NEXT:    mov pc, lr
   %sel = select i1 %cond, i32 0, i32 -1
   ret i32 %sel
 }
 
 define i32 @select_0_or_neg1_alt(i1 %cond) {
 ; CHECK-LABEL: select_0_or_neg1_alt:
 ; CHECK:       @ BB#0:
-; CHECK-NEXT:    mov r1, #1
-; CHECK-NEXT:    bic r0, r1, r0
-; CHECK-NEXT:    rsb r0, r0, #0
+; CHECK-NEXT:    and r0, r0, #1
+; CHECK-NEXT:    sub r0, r0, #1
 ; CHECK-NEXT:    mov pc, lr
   %z = zext i1 %cond to i32
   %add = add i32 %z, -1
   ret i32 %add
 }
 
 define i32 @select_0_or_neg1_alt_zeroext(i1 zeroext %cond) {
 ; CHECK-LABEL: select_0_or_neg1_alt_zeroext:
 ; CHECK:       @ BB#0:
-; CHECK-NEXT:    eor r0, r0, #1
-; CHECK-NEXT:    rsb r0, r0, #0
+; CHECK-NEXT:    sub r0, r0, #1
 ; CHECK-NEXT:    mov pc, lr
   %z = zext i1 %cond to i32
   %add = add i32 %z, -1
   ret i32 %add
 }
 
 define i32 @select_0_or_neg1_alt_signext(i1 signext %cond) {
 ; CHECK-LABEL: select_0_or_neg1_alt_signext: