[X86] Teach "(select (and (x , 0x1) == 0), y, (z | y) ) -> (-(and (x , 0x1)) & z ) | y" to also handle the case where there is no setcc and the and is used directly with the args swapped

[craig.topper]

Summary

It seems SimplifySetcc likes to turns (i1 (setcc ne (and x, 1), 1)) into (i1 (setcc eq (i1 (trunc x)), 0) which becomes (i1 (xor (i1 (trunc x)), 1)). If its used by a select, the xor gets removed by swapping the true/false values. Then type legalization comes along and turns the trunc into (i8 (and x, 1)). So in the end we have no setcc.

I attempted to make (i1 (setcc eq (and x, 1), 0)) also produce (i1 (xor (i1 (trunc x)), 1) so they would be the same, but discovered that broke this code.

This patch teachs the code in LowerSELECT to handle this alternate possibility. I moved the code above the call to LowerSETCC because I don't think we need to do that before doing this check.

Ultimately, this is step towards improving this code. Which currently produces a BT and SETAE instruction. But produces arithmetic/logic operations if you use "icmp ne i64 %a17, 1" or if you use "icmp ne i64 %a17, 0" and swap the select operands.

define void @foo(i64 %x, i32 %c.0282.in, i32 %d.0280, i32* %ptr0, i32* %ptr1) {
    %c.0282 = and i32 %c.0282.in, 268435455
    %a16 = lshr i64 32508, %x
    %a17 = and i64 %a16, 1
    %tobool = icmp eq i64 %a17, 0
    %. = select i1 %tobool, i32 1, i32 2
    %.286 = select i1 %tobool, i32 27, i32 26
    %shr97 = lshr i32 %c.0282, %.
    %shl98 = shl i32 %c.0282.in, %.286
    %or99 = or i32 %shr97, %shl98
    %shr100 = lshr i32 %d.0280, %.
    %shl101 = shl i32 %d.0280, %.286
    %or102 = or i32 %shr100, %shl101
    store i32 %or99, i32* %ptr0
    store i32 %or102, i32* %ptr1
    ret void
}

[craig.topper]

This is the complete patch. The previous one was relative to an intermediate step.

[spatel]

I was working on a generic DAGCombiner patch for what I think are the early patterns in the motivating case:
https://rise4fun.com/Alive/Gsyp

...but we might need multiple patches to fix this up. I'll post what I have and see what you think

[spatel]

Oops - I included the wrong link in the commit message for rL335433; that was meant to correspond to D48508.

[spatel]

Not sure how to make Phab select the previous version of the patch.
It's showing the patch from rL335433 now, which is not what we want of course.
Did D48508 affect any of the tests or motivation for this one?

[craig.topper]

D48508 fixed part my motivating case and I think there are other things we can do to fix the remaining issue. Abandoning this.

lib/Target/X86/X86ISelLowering.cpp

if (VT == MVT::v4i1 || VT == MVT::v2i1) {
     Op2 = DAG.getNode(ISD::INSERT_SUBVECTOR, DL, MVT::v8i1,
                       DAG.getUNDEF(MVT::v8i1), Op2, zeroConst);
     SDValue newSelect = DAG.getSelect(DL, MVT::v8i1, Cond, Op1, Op2);
     return DAG.getNode(ISD::EXTRACT_SUBVECTOR, DL, VT, newSelect, zeroConst);
   }
 
   // (select (and (x , 0x1) == 0), y, (z ^ y) ) -> (-(and (x , 0x1)) & z ) ^ y
   // (select (and (x , 0x1) == 0), y, (z | y) ) -> (-(and (x , 0x1)) & z ) | y
-  if (!Subtarget.hasCMov() && Cond.getOpcode() == ISD::SETCC &&
+  // (select (and (x , 0x1)), (z ^ y), y ) -> (-(and (x , 0x1)) & z ) ^ y
+  // (select (and (x , 0x1)), (z | y), y ) -> (-(and (x , 0x1)) & z ) | y
+  // We have to handle two cases of condition, setcc or a and used directly.
+  if (!Subtarget.hasCMov() &&
+      (Cond.getOpcode() == ISD::AND ||
+       (Cond.getOpcode() == ISD::SETCC &&
         cast<CondCodeSDNode>(Cond.getOperand(2))->get() == ISD::SETEQ &&
-      isNullConstant(Cond.getOperand(1)) &&
-      Cond.getOperand(0).getOpcode() == ISD::AND &&
-      isOneConstant(Cond.getOperand(0).getOperand(1))) {
-    SDValue And = Cond.getOperand(0);
+        isNullConstant(Cond.getOperand(1))))) {
+    SDValue And = Cond;
+    SDValue TrueVal = Op1;
+    SDValue FalseVal = Op2;
+    // If we matched a SETCC, look through it.
+    if (Cond.getOpcode() == ISD::SETCC) {
+      And = Cond.getOperand(0);
+      std::swap(TrueVal, FalseVal);
+    }
 
+    if (And.getOpcode() == ISD::AND && isOneConstant(And.getOperand(1))) {
       // true if FalseVal is XOR or OR operator and one of its operands
       // is equal to TrueVal
-    // ( a , a op b) || ( b , a op b)
+      // ( a op b, a) || ( a op b, b)
       auto isOrXorPattern = [](SDValue TrueVal, SDValue FalseVal,
                                SDValue &Src1, SDValue &Src2) {
-      if ((FalseVal.getOpcode() == ISD::XOR ||
-           FalseVal.getOpcode() == ISD::OR) &&
-          (FalseVal.getOperand(0) == TrueVal ||
-           FalseVal.getOperand(1) == TrueVal)) {
-        Src1 = FalseVal.getOperand(0) == TrueVal ? FalseVal.getOperand(1)
-                                                 : FalseVal.getOperand(0);
-        Src2 = TrueVal;
+        if ((TrueVal.getOpcode() == ISD::XOR ||
+             TrueVal.getOpcode() == ISD::OR) &&
+            (TrueVal.getOperand(0) == FalseVal ||
+             TrueVal.getOperand(1) == FalseVal)) {
+          Src1 = TrueVal.getOperand(0) == FalseVal ? TrueVal.getOperand(1)
+                                                   : TrueVal.getOperand(0);
+          Src2 = FalseVal;
           return true;
         }
         return false;
       };
 
       SDValue Src1, Src2;
-    if (isOrXorPattern(Op1, Op2, Src1, Src2)) {
+      if (isOrXorPattern(TrueVal, FalseVal, Src1, Src2)) {
         unsigned CmpSz = And.getSimpleValueType().getSizeInBits();
         // we need mask of all zeros or ones with same size of the other
         // operands.
         if (CmpSz > VT.getSizeInBits())
           And = DAG.getNode(ISD::TRUNCATE, DL, VT, And);
         else if (CmpSz < VT.getSizeInBits())
           And = DAG.getNode(ISD::AND, DL, VT,
               DAG.getNode(ISD::ANY_EXTEND, DL, VT, And.getOperand(0)),
               DAG.getConstant(1, DL, VT));
         SDValue Mask = DAG.getNode(ISD::SUB, DL, VT, DAG.getConstant(0, DL, VT),
                                    And); // -(and (x, 0x1))
         Mask = DAG.getNode(ISD::AND, DL, VT, Mask, Src1); // Mask & z
-      return DAG.getNode(Op2.getOpcode(), DL, VT, Mask, Src2);  // And Op y
+        return DAG.getNode(TrueVal.getOpcode(), DL, VT, Mask, Src2);  // And Op y
+      }
     }
   }
 
   if (Cond.getOpcode() == ISD::SETCC) {
     if (SDValue NewCond = LowerSETCC(Cond, DAG)) {
       Cond = NewCond;
       // If the condition was updated, it's possible that the operands of the
       // select were also updated (for example, EmitTest has a RAUW). Refresh

test/CodeGen/X86/select.ll

 ; CHECK-NEXT:    xorl $43, %eax
 ; CHECK-NEXT:    testb $1, %sil
 ; CHECK-NEXT:    cmovel %edi, %eax
 ; CHECK-NEXT:    ## kill: def $ax killed $ax killed $eax
 ; CHECK-NEXT:    retq
 ;
 ; MCU-LABEL: select_xor_1b:
 ; MCU:       # %bb.0: # %entry
-; MCU-NEXT:    testb $1, %dl
-; MCU-NEXT:    je .LBB26_2
-; MCU-NEXT:  # %bb.1:
-; MCU-NEXT:    xorl $43, %eax
-; MCU-NEXT:  .LBB26_2: # %entry
+; MCU-NEXT:    andl $1, %edx
+; MCU-NEXT:    negl %edx
+; MCU-NEXT:    andl $43, %edx
+; MCU-NEXT:    xorl %edx, %eax
 ; MCU-NEXT:    # kill: def $ax killed $ax killed $eax
 ; MCU-NEXT:    retl
 entry:
  %and = and i8 %cond, 1
  %cmp10 = icmp ne i8 %and, 1
  %0 = xor i16 %A, 43
  %1 = select i1 %cmp10, i16 %A, i16 %0
  ret i16 %1

 ; CHECK-NEXT:    xorl %edi, %esi
 ; CHECK-NEXT:    testb $1, %dl
 ; CHECK-NEXT:    cmovel %edi, %esi
 ; CHECK-NEXT:    movl %esi, %eax
 ; CHECK-NEXT:    retq
 ;
 ; MCU-LABEL: select_xor_2b:
 ; MCU:       # %bb.0: # %entry
-; MCU-NEXT:    testb $1, %cl
-; MCU-NEXT:    je .LBB28_2
-; MCU-NEXT:  # %bb.1:
-; MCU-NEXT:    xorl %edx, %eax
-; MCU-NEXT:  .LBB28_2: # %entry
+; MCU-NEXT:    andl $1, %ecx
+; MCU-NEXT:    negl %ecx
+; MCU-NEXT:    andl %edx, %ecx
+; MCU-NEXT:    xorl %ecx, %eax
 ; MCU-NEXT:    retl
 entry:
  %and = and i8 %cond, 1
  %cmp10 = icmp ne i8 %and, 1
  %0 = xor i32 %B, %A
  %1 = select i1 %cmp10, i32 %A, i32 %0
  ret i32 %1
 }

 ; CHECK-NEXT:    orl %edi, %esi
 ; CHECK-NEXT:    testb $1, %dl
 ; CHECK-NEXT:    cmovel %edi, %esi
 ; CHECK-NEXT:    movl %esi, %eax
 ; CHECK-NEXT:    retq
 ;
 ; MCU-LABEL: select_or_b:
 ; MCU:       # %bb.0: # %entry
-; MCU-NEXT:    testb $1, %cl
-; MCU-NEXT:    je .LBB30_2
-; MCU-NEXT:  # %bb.1:
-; MCU-NEXT:    orl %edx, %eax
-; MCU-NEXT:  .LBB30_2: # %entry
+; MCU-NEXT:    andl $1, %ecx
+; MCU-NEXT:    negl %ecx
+; MCU-NEXT:    andl %edx, %ecx
+; MCU-NEXT:    orl %ecx, %eax
 ; MCU-NEXT:    retl
 entry:
  %and = and i8 %cond, 1
  %cmp10 = icmp ne i8 %and, 1
  %0 = or i32 %B, %A
  %1 = select i1 %cmp10, i32 %A, i32 %0
  ret i32 %1
 }

 ; CHECK-NEXT:    orl %edi, %esi
 ; CHECK-NEXT:    testb $1, %dl
 ; CHECK-NEXT:    cmovel %edi, %esi
 ; CHECK-NEXT:    movl %esi, %eax
 ; CHECK-NEXT:    retq
 ;
 ; MCU-LABEL: select_or_1b:
 ; MCU:       # %bb.0: # %entry
-; MCU-NEXT:    testb $1, %cl
-; MCU-NEXT:    je .LBB32_2
-; MCU-NEXT:  # %bb.1:
-; MCU-NEXT:    orl %edx, %eax
-; MCU-NEXT:  .LBB32_2: # %entry
+; MCU-NEXT:    andl $1, %ecx
+; MCU-NEXT:    negl %ecx
+; MCU-NEXT:    andl %edx, %ecx
+; MCU-NEXT:    orl %ecx, %eax
 ; MCU-NEXT:    retl
 entry:
  %and = and i32 %cond, 1
  %cmp10 = icmp ne i32 %and, 1
  %0 = or i32 %B, %A
  %1 = select i1 %cmp10, i32 %A, i32 %0
  ret i32 %1
 }