[InstCombine] Simplify and merge FoldOrWithConstants/FoldXorWithConstants

[craig.topper]

Summary

The functions seem overly complicated. The body of this function is rechecking for an And operation to find the constant, but we already know we were looking at two Ands ORed together and the pieces are in variables. So just pass the correct pieces and check for two Constants.

Also since the two functions are almost identical, just pass in the opcode for the one difference so they can be combined.

Next step is to use m_APInt instead of ConstantInt.

[spatel]

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp

1833–1834

Dropped the null check on CI2? This will crash:

define i32 @D36439(i32 %a, i32 %b, i32 %c) {
  %tmp = or i32 %b, %a
  %tmp1 = and i32 %tmp, 1
  %tmp2 = and i32 %b, %c
  %tmp3 = or i32 %tmp1, %tmp2
  ret i32 %tmp3
}

[craig.topper]

Add null check for second ConstantInt.

[craig.topper]

Remove a change that accidentally crept into my last diff.

[spatel]

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp

1968–1972

It looks functionally correct now, but I'm finding it hard to follow the matchers because we do half of it down here and then finish up in the helper. It also doesn't help that the comment formula variables don't line up with the code ('A' in the formula above is 'V1' in the code?). Can we just match the constants down here and get rid of the helper? Or move these initial matchers into the helper?

[craig.topper]

Remove the helper and move the code into an earlier block that already checked for constants.

[spatel]

LGTM - thanks!

lib/Target/X86/X86InstrFragmentsSIMD.td

def masked_truncstore_s_vi32 : PatFrag<(ops node:$src1, node:$src2, node:$src3),
                                (X86MTruncSStore node:$src1, node:$src2, node:$src3), [{
   return cast<MaskedTruncSStoreSDNode>(N)->getMemoryVT().getScalarType() == MVT::i32;
 }]>;
 
 def masked_truncstore_us_vi32 : PatFrag<(ops node:$src1, node:$src2, node:$src3),
                                (X86MTruncUSStore node:$src1, node:$src2, node:$src3), [{
   return cast<MaskedTruncUSStoreSDNode>(N)->getMemoryVT().getScalarType() == MVT::i32;
 }]>;
-
-def assertzext_i1 :
-  PatFrag<(ops node:$src), (assertzext node:$src), [{
-    return cast<VTSDNode>(N->getOperand(1))->getVT() == MVT::i1;
-}]>;

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp

Value *InstCombiner::foldOrOfFCmps(FCmpInst *LHS, FCmpInst *RHS) {
   }
 
   return nullptr;
 }
 
 /// This helper function folds:
 ///
 ///     ((A | B) & C1) | (B & C2)
+/// or:
+///     ((A ^ B) & C1) | (B & C2)
 ///
 /// into:
 ///
 ///     (A & C1) | B
+/// or:
+///     (A & C1) ^ B
 ///
 /// when the XOR of the two constants is "all ones" (-1).
-static Instruction *FoldOrWithConstants(BinaryOperator &I, Value *Op,
-                                        Value *A, Value *B, Value *C,
+static Instruction *FoldOrXorWithConstants(Instruction::BinaryOps Opc, Value *A,
+                                           Value *B, Value *C1, Value *C2,
                                            InstCombiner::BuilderTy &Builder) {
-  ConstantInt *CI1 = dyn_cast<ConstantInt>(C);
+  ConstantInt *CI1 = dyn_cast<ConstantInt>(C1);
   if (!CI1) return nullptr;
 
-  Value *V1 = nullptr;
-  ConstantInt *CI2 = nullptr;
-  if (!match(Op, m_And(m_Value(V1), m_ConstantInt(CI2)))) return nullptr;
+  ConstantInt *CI2 = dyn_cast<ConstantInt>(C2);
+  if (!C2) return nullptr;
 
   APInt Xor = CI1->getValue() ^ CI2->getValue();
   if (!Xor.isAllOnesValue()) return nullptr;
 
-  if (V1 == A || V1 == B) {
-    Value *NewOp = Builder.CreateAnd((V1 == A) ? B : A, CI1);
-    return BinaryOperator::CreateOr(NewOp, V1);
-  }
-
-  return nullptr;
-}
-
-/// \brief This helper function folds:
-///
-///     ((A ^ B) & C1) | (B & C2)
-///
-/// into:
-///
-///     (A & C1) ^ B
-///
-/// when the XOR of the two constants is "all ones" (-1).
-static Instruction *FoldXorWithConstants(BinaryOperator &I, Value *Op,
-                                         Value *A, Value *B, Value *C,
-                                         InstCombiner::BuilderTy &Builder) {
-  ConstantInt *CI1 = dyn_cast<ConstantInt>(C);
-  if (!CI1)
-    return nullptr;
-
-  Value *V1 = nullptr;
-  ConstantInt *CI2 = nullptr;
-  if (!match(Op, m_And(m_Value(V1), m_ConstantInt(CI2))))
-    return nullptr;
-
-  APInt Xor = CI1->getValue() ^ CI2->getValue();
-  if (!Xor.isAllOnesValue())
-    return nullptr;
-
-  if (V1 == A || V1 == B) {
-    Value *NewOp = Builder.CreateAnd(V1 == A ? B : A, CI1);
-    return BinaryOperator::CreateXor(NewOp, V1);
-  }
-
-  return nullptr;
+  Value *NewOp = Builder.CreateAnd(A, C1);
+  return BinaryOperator::Create(Opc, NewOp, B);
 }
 
 // FIXME: We use commutative matchers (m_c_*) for some, but not all, matches
 // here. We should standardize that construct where it is needed or choose some
 // other way to ensure that commutated variants of patterns are not missed.
 Instruction *InstCombiner::visitOr(BinaryOperator &I) {
   bool Changed = SimplifyAssociativeOrCommutative(I);
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
if (Op0->hasOneUse() || Op1->hasOneUse()) {
       if (Value *V = matchSelectFromAndOr(B, D, C, A, Builder))
         return replaceInstUsesWith(I, V);
       if (Value *V = matchSelectFromAndOr(D, B, A, C, Builder))
         return replaceInstUsesWith(I, V);
       if (Value *V = matchSelectFromAndOr(D, B, C, A, Builder))
         return replaceInstUsesWith(I, V);
     }
 
     // ((A|B)&1)|(B&-2) -> (A&1) | B
     if (match(A, m_c_Or(m_Value(V1), m_Specific(B)))) {
-      if (Instruction *Ret = FoldOrWithConstants(I, Op1, V1, B, C, Builder))
+      if (Instruction *Ret = FoldOrXorWithConstants(BinaryOperator::Or,
+                                                    V1, B, C, D, Builder))
         return Ret;
     }
     // (B&-2)|((A|B)&1) -> (A&1) | B
     if (match(B, m_c_Or(m_Specific(A), m_Value(V1)))) {
-      if (Instruction *Ret = FoldOrWithConstants(I, Op0, A, V1, D, Builder))
+      if (Instruction *Ret = FoldOrXorWithConstants(BinaryOperator::Or,
+                                                    V1, A, D, C, Builder))
         return Ret;
     }
     // ((A^B)&1)|(B&-2) -> (A&1) ^ B
     if (match(A, m_c_Xor(m_Value(V1), m_Specific(B)))) {
-      if (Instruction *Ret = FoldXorWithConstants(I, Op1, V1, B, C, Builder))
+      if (Instruction *Ret = FoldOrXorWithConstants(BinaryOperator::Xor,
+                                                    V1, B, C, D, Builder))
         return Ret;
     }
     // (B&-2)|((A^B)&1) -> (A&1) ^ B
     if (match(B, m_c_Xor(m_Specific(A), m_Value(V1)))) {
-      if (Instruction *Ret = FoldXorWithConstants(I, Op0, A, V1, D, Builder))
+      if (Instruction *Ret = FoldOrXorWithConstants(BinaryOperator::Xor,
+                                                    V1, A, D, C, Builder))
         return Ret;
     }
   }
 
   // (A ^ B) | ((B ^ C) ^ A) -> (A ^ B) | C
   if (match(Op0, m_Xor(m_Value(A), m_Value(B))))
     if (match(Op1, m_Xor(m_Xor(m_Specific(B), m_Value(C)), m_Specific(A))))
       return BinaryOperator::CreateOr(Op0, C);