[InstCombine] use commutative matchers for patterns with commutative operators

Background/motivation - I was circling back around to:
https://llvm.org/bugs/show_bug.cgi?id=28296

I made a simple patch for that and noticed some regressions, so added test cases for
those with rL281055, and this is hopefully the minimal fix for just those cases.

But as you can see from the surrounding untouched folds, we are missing commuted patterns
all over the place, and of course there are no regression tests to cover any of those cases.

We could sprinkle "m_c_" dust all over this file and catch most of the missing folds, but
then we still wouldn't have test coverage, and we'd still miss some fraction of commuted
patterns because they require adjustments to the match order.

I'm aware of the concern about the potential compile-time performance impact of adding
matches like this (currently being discussed on llvm-dev), but I don't think there's any
evidence yet to suggest that handling commutative pattern matching more thoroughly is not
a worthwhile goal of InstCombine.

Differential Revision: https://reviews.llvm.org/D24419

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@290067 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index e1e060b283efc8a1a042e886655f794260597d37..cbcd459f582a6d296a55fb399c325916ee7d6b61 100644 (file)
--- a/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -1345,6 +1345,9 @@ static Instruction *foldBoolSextMaskToSelect(BinaryOperator &I) {
   return nullptr;
 }
 
+// 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::visitAnd(BinaryOperator &I) {
   bool Changed = SimplifyAssociativeOrCommutative(I);
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
@@ -1543,8 +1546,9 @@ Instruction *InstCombiner::visitAnd(BinaryOperator &I) {
       return BinaryOperator::CreateAnd(A, B);
 
     // ((~A) ^ B) & (A | B) -> (A & B)
+    // ((~A) ^ B) & (B | A) -> (A & B)
     if (match(Op0, m_Xor(m_Not(m_Value(A)), m_Value(B))) &&
-        match(Op1, m_Or(m_Specific(A), m_Specific(B))))
+        match(Op1, m_c_Or(m_Specific(A), m_Specific(B))))
       return BinaryOperator::CreateAnd(A, B);
   }
 
@@ -2161,6 +2165,9 @@ Instruction *InstCombiner::FoldXorWithConstants(BinaryOperator &I, Value *Op,
   return nullptr;
 }
 
+// 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);
@@ -2250,8 +2257,9 @@ Instruction *InstCombiner::visitOr(BinaryOperator &I) {
       match(Op1, m_Not(m_Specific(A))))
     return BinaryOperator::CreateOr(Builder->CreateNot(A), B);
 
-  // (A & (~B)) | (A ^ B) -> (A ^ B)
-  if (match(Op0, m_And(m_Value(A), m_Not(m_Value(B)))) &&
+  // (A & ~B) | (A ^ B) -> (A ^ B)
+  // (~B & A) | (A ^ B) -> (A ^ B)
+  if (match(Op0, m_c_And(m_Value(A), m_Not(m_Value(B)))) &&
       match(Op1, m_Xor(m_Specific(A), m_Specific(B))))
     return BinaryOperator::CreateXor(A, B);
 
@@ -2427,14 +2435,15 @@ Instruction *InstCombiner::visitOr(BinaryOperator &I) {
         return BinaryOperator::CreateOr(Not, Op0);
       }
 
-  // (A & B) | ((~A) ^ B) -> (~A ^ B)
-  if (match(Op0, m_And(m_Value(A), m_Value(B))) &&
-      match(Op1, m_Xor(m_Not(m_Specific(A)), m_Specific(B))))
-    return BinaryOperator::CreateXor(Builder->CreateNot(A), B);
-
-  // ((~A) ^ B) | (A & B) -> (~A ^ B)
-  if (match(Op0, m_Xor(m_Not(m_Value(A)), m_Value(B))) &&
-      match(Op1, m_And(m_Specific(A), m_Specific(B))))
+  // (A & B) | (~A ^ B) -> (~A ^ B)
+  // (A & B) | (B ^ ~A) -> (~A ^ B)
+  // (B & A) | (~A ^ B) -> (~A ^ B)
+  // (B & A) | (B ^ ~A) -> (~A ^ B)
+  // The match order is important: match the xor first because the 'not'
+  // operation defines 'A'. We do not need to match the xor as Op0 because the
+  // xor was canonicalized to Op1 above.
+  if (match(Op1, m_c_Xor(m_Not(m_Value(A)), m_Value(B))) &&
+      match(Op0, m_c_And(m_Specific(A), m_Specific(B))))
     return BinaryOperator::CreateXor(Builder->CreateNot(A), B);
 
   if (SwappedForXor)
@@ -2514,6 +2523,9 @@ Instruction *InstCombiner::visitOr(BinaryOperator &I) {
   return Changed ? &I : nullptr;
 }
 
+// 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::visitXor(BinaryOperator &I) {
   bool Changed = SimplifyAssociativeOrCommutative(I);
   Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1);
@@ -2736,20 +2748,22 @@ Instruction *InstCombiner::visitXor(BinaryOperator &I) {
         return BinaryOperator::CreateXor(A, B);
     }
     // (A | ~B) ^ (~A | B) -> A ^ B
-    if (match(Op0I, m_Or(m_Value(A), m_Not(m_Value(B)))) &&
-        match(Op1I, m_Or(m_Not(m_Specific(A)), m_Specific(B)))) {
+    // (~B | A) ^ (~A | B) -> A ^ B
+    if (match(Op0I, m_c_Or(m_Value(A), m_Not(m_Value(B)))) &&
+        match(Op1I, m_Or(m_Not(m_Specific(A)), m_Specific(B))))
       return BinaryOperator::CreateXor(A, B);
-    }
+
     // (~A | B) ^ (A | ~B) -> A ^ B
     if (match(Op0I, m_Or(m_Not(m_Value(A)), m_Value(B))) &&
         match(Op1I, m_Or(m_Specific(A), m_Not(m_Specific(B))))) {
       return BinaryOperator::CreateXor(A, B);
     }
     // (A & ~B) ^ (~A & B) -> A ^ B
-    if (match(Op0I, m_And(m_Value(A), m_Not(m_Value(B)))) &&
-        match(Op1I, m_And(m_Not(m_Specific(A)), m_Specific(B)))) {
+    // (~B & A) ^ (~A & B) -> A ^ B
+    if (match(Op0I, m_c_And(m_Value(A), m_Not(m_Value(B)))) &&
+        match(Op1I, m_And(m_Not(m_Specific(A)), m_Specific(B))))
       return BinaryOperator::CreateXor(A, B);
-    }
+
     // (~A & B) ^ (A & ~B) -> A ^ B
     if (match(Op0I, m_And(m_Not(m_Value(A)), m_Value(B))) &&
         match(Op1I, m_And(m_Specific(A), m_Not(m_Specific(B))))) {
@@ -2785,9 +2799,10 @@ Instruction *InstCombiner::visitXor(BinaryOperator &I) {
       return BinaryOperator::CreateOr(A, B);
   }
 
-  Value *A = nullptr, *B = nullptr;
-  // (A & ~B) ^ (~A) -> ~(A & B)
-  if (match(Op0, m_And(m_Value(A), m_Not(m_Value(B)))) &&
+  // (A & ~B) ^ ~A -> ~(A & B)
+  // (~B & A) ^ ~A -> ~(A & B)
+  Value *A, *B;
+  if (match(Op0, m_c_And(m_Value(A), m_Not(m_Value(B)))) &&
       match(Op1, m_Not(m_Specific(A))))
     return BinaryOperator::CreateNot(Builder->CreateAnd(A, B));
 

diff --git a/test/Transforms/InstCombine/or-xor.ll b/test/Transforms/InstCombine/or-xor.ll
index 3ba51de7108bebe58acd3dfcfeca1df1d4e8927e..ec5b71656a476a669f82f37618b18d9600dc4610 100644 (file)
--- a/test/Transforms/InstCombine/or-xor.ll
+++ b/test/Transforms/InstCombine/or-xor.ll
@@ -140,14 +140,9 @@ define i32 @test12(i32 %x, i32 %y) {
   ret i32 %and
 }
 
-; FIXME: We miss the fold because the pattern matching is inadequate.
-
 define i32 @test12_commuted(i32 %x, i32 %y) {
 ; CHECK-LABEL: @test12_commuted(
-; CHECK-NEXT:    [[NEG:%.*]] = xor i32 %x, -1
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[NEG]], %y
-; CHECK-NEXT:    [[OR:%.*]] = or i32 %y, %x
-; CHECK-NEXT:    [[AND:%.*]] = and i32 [[XOR]], [[OR]]
+; CHECK-NEXT:    [[AND:%.*]] = and i32 %x, %y
 ; CHECK-NEXT:    ret i32 [[AND]]
 ;
   %neg = xor i32 %x, -1
@@ -183,15 +178,9 @@ define i32 @test14(i32 %x, i32 %y) {
   ret i32 %xor
 }
 
-; FIXME: We miss the fold because the pattern matching is inadequate.
-
 define i32 @test14_commuted(i32 %x, i32 %y) {
 ; CHECK-LABEL: @test14_commuted(
-; CHECK-NEXT:    [[NOTY:%.*]] = xor i32 %y, -1
-; CHECK-NEXT:    [[NOTX:%.*]] = xor i32 %x, -1
-; CHECK-NEXT:    [[OR1:%.*]] = or i32 [[NOTY]], %x
-; CHECK-NEXT:    [[OR2:%.*]] = or i32 [[NOTX]], %y
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[OR1]], [[OR2]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 %x, %y
 ; CHECK-NEXT:    ret i32 [[XOR]]
 ;
   %noty = xor i32 %y, -1
@@ -216,15 +205,9 @@ define i32 @test15(i32 %x, i32 %y) {
   ret i32 %xor
 }
 
-; FIXME: We miss the fold because the pattern matching is inadequate.
-
 define i32 @test15_commuted(i32 %x, i32 %y) {
 ; CHECK-LABEL: @test15_commuted(
-; CHECK-NEXT:    [[NOTY:%.*]] = xor i32 %y, -1
-; CHECK-NEXT:    [[NOTX:%.*]] = xor i32 %x, -1
-; CHECK-NEXT:    [[AND1:%.*]] = and i32 [[NOTY]], %x
-; CHECK-NEXT:    [[AND2:%.*]] = and i32 [[NOTX]], %y
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[AND1]], [[AND2]]
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 %x, %y
 ; CHECK-NEXT:    ret i32 [[XOR]]
 ;
   %noty = xor i32 %y, -1

diff --git a/test/Transforms/InstCombine/or.ll b/test/Transforms/InstCombine/or.ll
index 38b8ebc5adf9a97d5ceee7143bfa2e680257515a..facd63019fed94f8564ac51d6c660c3ebaf523ac 100644 (file)
--- a/test/Transforms/InstCombine/or.ll
+++ b/test/Transforms/InstCombine/or.ll
@@ -570,14 +570,10 @@ define i32 @test42(i32 %a, i32 %b) {
   ret i32 %or
 }
 
-; FIXME: We miss the fold because the pattern matching is inadequate.
-
 define i32 @test42_commuted_and(i32 %a, i32 %b) {
 ; CHECK-LABEL: @test42_commuted_and(
-; CHECK-NEXT:    [[NEGA:%.*]] = xor i32 %a, -1
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[NEGA]], %b
-; CHECK-NEXT:    [[AND:%.*]] = and i32 %b, %a
-; CHECK-NEXT:    [[OR:%.*]] = or i32 [[XOR]], [[AND]]
+; CHECK-NEXT:    [[TMP1:%.*]] = xor i32 %a, -1
+; CHECK-NEXT:    [[OR:%.*]] = xor i32 [[TMP1]], %b
 ; CHECK-NEXT:    ret i32 [[OR]]
 ;
   %nega = xor i32 %a, -1
@@ -587,14 +583,10 @@ define i32 @test42_commuted_and(i32 %a, i32 %b) {
   ret i32 %or
 }
 
-; FIXME: We miss the fold because the pattern matching is inadequate.
-
 define i32 @test42_commuted_xor(i32 %a, i32 %b) {
 ; CHECK-LABEL: @test42_commuted_xor(
-; CHECK-NEXT:    [[NEGA:%.*]] = xor i32 %a, -1
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 %b, [[NEGA]]
-; CHECK-NEXT:    [[AND:%.*]] = and i32 %a, %b
-; CHECK-NEXT:    [[OR:%.*]] = or i32 [[XOR]], [[AND]]
+; CHECK-NEXT:    [[TMP1:%.*]] = xor i32 %a, -1
+; CHECK-NEXT:    [[OR:%.*]] = xor i32 [[TMP1]], %b
 ; CHECK-NEXT:    ret i32 [[OR]]
 ;
   %nega = xor i32 %a, -1
@@ -618,14 +610,9 @@ define i32 @test43(i32 %a, i32 %b) {
   ret i32 %or
 }
 
-; FIXME: We miss the fold because the pattern matching is inadequate.
-
 define i32 @test43_commuted_and(i32 %a, i32 %b) {
 ; CHECK-LABEL: @test43_commuted_and(
-; CHECK-NEXT:    [[NEG:%.*]] = xor i32 %b, -1
-; CHECK-NEXT:    [[AND:%.*]] = and i32 [[NEG]], %a
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 %a, %b
-; CHECK-NEXT:    [[OR:%.*]] = or i32 [[AND]], [[XOR]]
+; CHECK-NEXT:    [[OR:%.*]] = xor i32 %a, %b
 ; CHECK-NEXT:    ret i32 [[OR]]
 ;
   %neg = xor i32 %b, -1

diff --git a/test/Transforms/InstCombine/xor2.ll b/test/Transforms/InstCombine/xor2.ll
index 7ec84d8caf9455e68ae89d15b9e3634bd4f1e558..f3591ed9c8a9b5e86dded7ff0ab06ab66982ff1c 100644 (file)
--- a/test/Transforms/InstCombine/xor2.ll
+++ b/test/Transforms/InstCombine/xor2.ll
@@ -180,14 +180,10 @@ define i32 @test12(i32 %a, i32 %b) {
   ret i32 %xor
 }
 
-; FIXME: We miss the fold because the pattern matching is inadequate.
-
 define i32 @test12commuted(i32 %a, i32 %b) {
 ; CHECK-LABEL: @test12commuted(
-; CHECK-NEXT:    [[NEGB:%.*]] = xor i32 %b, -1
-; CHECK-NEXT:    [[AND:%.*]] = and i32 [[NEGB]], %a
-; CHECK-NEXT:    [[NEGA:%.*]] = xor i32 %a, -1
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[AND]], [[NEGA]]
+; CHECK-NEXT:    [[TMP1:%.*]] = and i32 %a, %b
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[TMP1]], -1
 ; CHECK-NEXT:    ret i32 [[XOR]]
 ;
   %negb = xor i32 %b, -1
@@ -198,7 +194,7 @@ define i32 @test12commuted(i32 %a, i32 %b) {
 }
 
 ; This is a test of canonicalization via operand complexity.
-; The final xor has a binary operator and a (fake) unary operator, 
+; The final xor has a binary operator and a (fake) unary operator,
 ; so binary (more complex) should come first.
 
 define i32 @test13(i32 %a, i32 %b) {
@@ -214,14 +210,10 @@ define i32 @test13(i32 %a, i32 %b) {
   ret i32 %xor
 }
 
-; FIXME: We miss the fold because the pattern matching is inadequate.
-
 define i32 @test13commuted(i32 %a, i32 %b) {
 ; CHECK-LABEL: @test13commuted(
-; CHECK-NEXT:    [[NEGA:%.*]] = xor i32 %a, -1
-; CHECK-NEXT:    [[NEGB:%.*]] = xor i32 %b, -1
-; CHECK-NEXT:    [[AND:%.*]] = and i32 [[NEGB]], %a
-; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[AND]], [[NEGA]]
+; CHECK-NEXT:    [[TMP1:%.*]] = and i32 %a, %b
+; CHECK-NEXT:    [[XOR:%.*]] = xor i32 [[TMP1]], -1
 ; CHECK-NEXT:    ret i32 [[XOR]]
 ;
   %nega = xor i32 %a, -1