Fix optimisations of SELECT_CC which assumed result is boolean

[olista01]

Summary

Some optimisations in DAGCombiner cause miscompilations for targets that use TargetLowering::UndefinedBooleanContent, because they assume that the results of a SELECT_CC node are boolean values, and can be safely ANDed, ORed and XORed. These optimisations are only valid for targets that use ZeroOrOneBooleanContent or ZeroOrNegativeOneBooleanContent.

This is a follow-up to D6210/r221693.

[t.p.northover]

Hi Oliver,

Oh, nasty. Unfortunately I can't think of a way around the isSetCCEquivalent change. All the uses really do seem to break down. I don't think the !(setcc a, b) change is quite right though:

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3833

I think this should use isConstTrueVal instead of hard-coding either 1 or AllOnes.

[arsenm]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3833

Agree

[olista01]

Use isConstTrueVal instead of hard-coding either 1 or AllOnes.

[t.p.northover]

Hi Oliver,

Thanks for updating the patch. Nearly there, I think, but there's one improvement we can make:

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

3833

isConstTrueVal is actually a bit more general than this (or it would be if it didn't have a bug itself), handling BUILD_VECTOR as well as ConstantSDNode. So you can probably drop the N1C check and use N1 directly.

E.g.

define <2 x i64> @foo(<2 x i64> %lhs, <2 x i64> %rhs) {
  %tst = icmp eq <2 x i64> %lhs, %rhs
  %ntst = xor <2 x i1> %tst, <i1 1 , i1 undef>
  %btst = sext <2 x i1> %ntst to <2 x i64>
  ret <2 x i64> %btst
}

(The undef should be a 1, I think, but that bug in isConstTrueVal (& isConstFalseVal) means the optimisation triggers with undef instead).

[olista01]

Check N1 rather than N1C, so that this optimisation also works on vectors.

[t.p.northover]

Thanks Oliver, this looks fine.

Tim.

[arsenm]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

648–650

Is this really necessary? I believe isConstTrue should do the correct thing for for UndefinedBooleanContent

[t.p.northover]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

648–650

The problem is that most (all?) contexts this function is used in, the output isn't necessarily a boolean. E.g.

(and (select_cc L1, R1, 0x1230, 0x4561), (select_cc L2, R2, 0x7890, 0xabc1))

That's going to be a very specific constant, but any argument based on thinking of it as equivalent to

(and (setcc L1, R1), (setcc L2, R2))

is going to be on very shaky ground. Thinking about it, you could probably allow it if the type is still i1 or <N x i1>, but not more generally.

[olista01]

Thanks, committed revision 222123.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (N.getOpcode() == ISD::SETCC) {
     return true;
   }
 
   if (N.getOpcode() != ISD::SELECT_CC ||
       !TLI.isConstTrueVal(N.getOperand(2).getNode()) ||
       !TLI.isConstFalseVal(N.getOperand(3).getNode()))
     return false;
 
+  if (TLI.getBooleanContents(N.getValueType()) ==
+      TargetLowering::UndefinedBooleanContent)
+    return false;
+
   LHS = N.getOperand(0);
   RHS = N.getOperand(1);
   CC  = N.getOperand(4);
   return true;
 }
 
 /// Return true if this is a SetCC-equivalent operation with only one use.
 /// If this is true, it allows the users to invert the operation for free when
SDValue DAGCombiner::visitXOR(SDNode *N) {
   if (N1C && N1C->isNullValue())
     return N0;
   // reassociate xor
   SDValue RXOR = ReassociateOps(ISD::XOR, SDLoc(N), N0, N1);
   if (RXOR.getNode())
     return RXOR;
 
   // fold !(x cc y) -> (x !cc y)
-  if (N1C && N1C->getAPIntValue().isAllOnesValue() &&
-      isSetCCEquivalent(N0, LHS, RHS, CC)) {
+  if (TLI.isConstTrueVal(N1.getNode()) && isSetCCEquivalent(N0, LHS, RHS, CC)) {
     bool isInt = LHS.getValueType().isInteger();
     ISD::CondCode NotCC = ISD::getSetCCInverse(cast<CondCodeSDNode>(CC)->get(),
                                                isInt);
 
     if (!LegalOperations ||
         TLI.isCondCodeLegal(NotCC, LHS.getSimpleValueType())) {
       switch (N0.getOpcode()) {
       default:

test/CodeGen/ARM/select_xform.ll

 ; T2: subgt r0, #1
   %cmp = icmp sgt i32 %a, 10
   %conv = sext i1 %cmp to i32
   %add = add i32 %conv, %c
   ret i32 %add
 }
 
 ; Do not fold the xor into the select
-define i32 @t15(i32 %p1, i32 %p2, i32 %p3) {
+define i32 @t15(i32 %p) {
 entry:
-; ARM: cmp            r0, #8
-; ARM: mov{{(le|gt)}} [[REG:r[0-9]+]], {{r[0-9]+}}
-; ARM: eor            r0, [[REG]], #1
+; ARM-LABEL: t15:
+; ARM: mov     [[REG:r[0-9]+]], #2
+; ARM: cmp     r0, #8
+; ARM: movwgt  [[REG:r[0-9]+]], #1
+; ARM: eor     r0, [[REG:r[0-9]+]], #1
 
-; T2: cmp       r0, #8
-; T2: it        [[CC:(le|gt)]]
-; T2: mov[[CC]] [[REG:r[0-9]+]], {{r[0-9]+}}
-; T2: eor       r0, [[REG:r[0-9]+]], #1
-  %cmp = icmp sgt i32 %p1, 8
-  %a = select i1 %cmp, i32 %p2, i32 %p3
+; T2-LABEL: t15:
+; T2: movs    [[REG:r[0-9]+]], #2
+; T2: cmp     [[REG:r[0-9]+]], #8
+; T2: it      gt
+; T2: movgt   [[REG:r[0-9]+]], #1
+; T2: eor     r0, [[REG:r[0-9]+]], #1
+  %cmp = icmp sgt i32 %p, 8
+  %a = select i1 %cmp, i32 1, i32 2
   %xor = xor i32 %a, 1
   ret i32 %xor
 }
+
+define i32 @t16(i32 %x, i32 %y) {
+entry:
+; ARM-LABEL: t16:
+; ARM: and r0, {{r[0-9]+}}, {{r[0-9]+}}
+
+; T2-LABEL: t16:
+; T2: ands r0, {{r[0-9]+}}
+  %cmp = icmp eq i32 %x, 0
+  %cond = select i1 %cmp, i32 5, i32 2
+  %cmp1 = icmp eq i32 %y, 0
+  %cond2 = select i1 %cmp1, i32 3, i32 4
+  %and = and i32 %cond2, %cond
+  ret i32 %and
+}
+
+define i32 @t17(i32 %x, i32 %y) #0 {
+entry:
+; ARM-LABEL: t17:
+; ARM: and r0, {{r[0-9]+}}, {{r[0-9]+}}
+
+; T2-LABEL: t17:
+; T2: ands r0, {{r[0-9]+}}
+  %cmp = icmp eq i32 %x, -1
+  %cond = select i1 %cmp, i32 5, i32 2
+  %cmp1 = icmp eq i32 %y, -1
+  %cond2 = select i1 %cmp1, i32 3, i32 4
+  %and = and i32 %cond2, %cond
+  ret i32 %and
+}
+
+define i32 @t18(i32 %x, i32 %y) #0 {
+entry:
+; ARM-LABEL: t18:
+; ARM: and r0, {{r[0-9]+}}, {{r[0-9]+}}
+
+; T2-LABEL: t18:
+; T2: and.w r0, {{r[0-9]+}}
+  %cmp = icmp ne i32 %x, 0
+  %cond = select i1 %cmp, i32 5, i32 2
+  %cmp1 = icmp ne i32 %x, -1
+  %cond2 = select i1 %cmp1, i32 3, i32 4
+  %and = and i32 %cond2, %cond
+  ret i32 %and
+}
+
+define i32 @t19(i32 %x, i32 %y) #0 {
+entry:
+; ARM-LABEL: t19:
+; ARM: orr r0, {{r[0-9]+}}, {{r[0-9]+}}
+
+; T2-LABEL: t19:
+; T2: orrs r0, {{r[0-9]+}}
+  %cmp = icmp ne i32 %x, 0
+  %cond = select i1 %cmp, i32 5, i32 2
+  %cmp1 = icmp ne i32 %y, 0
+  %cond2 = select i1 %cmp1, i32 3, i32 4
+  %or = or i32 %cond2, %cond
+  ret i32 %or
+}
+
+define i32 @t20(i32 %x, i32 %y) #0 {
+entry:
+; ARM-LABEL: t20:
+; ARM: orr r0, {{r[0-9]+}}, {{r[0-9]+}}
+
+; T2-LABEL: t20:
+; T2: orrs r0, {{r[0-9]+}}
+  %cmp = icmp ne i32 %x, -1
+  %cond = select i1 %cmp, i32 5, i32 2
+  %cmp1 = icmp ne i32 %y, -1
+  %cond2 = select i1 %cmp1, i32 3, i32 4
+  %or = or i32 %cond2, %cond
+  ret i32 %or
+}
+
+define  <2 x i32> @t21(<2 x i32> %lhs, <2 x i32> %rhs) {
+; CHECK-LABEL: t21:
+; CHECK-NOT: eor
+; CHECK: mvn
+; CHECK-NOT: eor
+  %tst = icmp eq <2 x i32> %lhs, %rhs
+  %ntst = xor <2 x i1> %tst, <i1 1 , i1 undef>
+  %btst = sext <2 x i1> %ntst to <2 x i32>
+  ret <2 x i32> %btst
+}