[PATCH] D124119: [InstCombine] Combine instructions of type or/and where AND masks can be combined.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2963-2964
+  // (A | B) | (C & D)
+  if (match(Op0, m_Or(m_Value(A), m_Value(B))) &&
+      match(Op1, m_And(m_Value(C), m_Value(D)))) {
+    Value *X;
----------------
This is still missing one-use checks and tests. We should have tests where one or more of the intermediate values has some other use. Look in the test file that you are modifying for tests with `call void @use(i8)`.

If we are creating 3 instructions, then both Op0 and Op1 must have only one use to ensure that we do not end up with more instructions than we started with.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2966
+    Value *X;
+    // (C | (A & D)) | (A & B) -> C | ((A & D) | (A & B))
+    // (C | (D & A)) | (A & B) -> C | ((D & A) | (A & B))
----------------
The code comments are confusing - don't reuse 'D' if we've already specified that as the outer-level `and` instruction.

It seems better to match the final `or` with a commutative matcher rather than duplicate all of this code twice. It would be something like:
  if (match(&I, m_c_Or(m_And(m_Value(A), m_Value(B)), m_Or(m_Value(C), m_Value(D))))


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2970
+    // (C | (D & B)) | (A & B) -> C | ((D & B) | (A & B))
+    if (match(B, m_c_BinOp(Instruction::And, m_Specific(C), m_Value(X))) ||
+        match(B, m_c_BinOp(Instruction::And, m_Specific(D), m_Value(X))))
----------------
Why use m_c_BinOp rather than m_c_And?

If we are not using 'X', then there is no reason to capture it - just use plain `m_Value()`.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2972
+        match(B, m_c_BinOp(Instruction::And, m_Specific(D), m_Value(X))))
+      return BinaryOperator::CreateOr(A, Builder.CreateOr(B, Op1));
+    // ((A & D) | C) | (A & B) -> ((A & D) | (A & B)) | C
----------------
I think it would be better (more efficient) to create the optimal pattern directly rather than relying on subsequent folds to do it for us. We can create the or(and(or...)) sequence right here.


================
Comment at: llvm/test/Transforms/InstCombine/and-or.ll:384
-; CHECK-NEXT:    [[AND2:%.*]] = and i8 [[A]], [[D:%.*]]
-; CHECK-NEXT:    [[OR1:%.*]] = or i8 [[AND2]], [[C:%.*]]
-; CHECK-NEXT:    [[OR2:%.*]] = or i8 [[OR1]], [[AND1]]
----------------
Notice that pat1-4 are canonicalized to the same form as pat5-8, so these are not testing the patterns that you intended. 

You'll need to add an extra instruction to these tests (and also pat1-4 in the next set of tests) to maintain `%c` as operand 0 of the `or`. 

Search for "thwart complexity-based canonicalization" in the InstCombine test dir for examples of how to do this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124119/new/

https://reviews.llvm.org/D124119