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

[Biplob Mishra via Phabricator via llvm-commits]

bipmis added inline comments.


================
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))
----------------
spatel wrote:
> 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))))
Ya the method and the comment is based on the current implementation where we do not actually reduce number of instructions, but reorder such that the AND's are now ORed and the existing implementation in InstCombine tryFactorization() folds them and reduces the instructions.


================
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))))
----------------
spatel wrote:
> 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()`.
m_c_BinOp was used as it can look for 2 operands in either order which is needed and could reduce extra code. 


================
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
----------------
spatel wrote:
> 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.
Right. My point of view is if there is an existing implementation in the instruction combine to do this, should we redo the same here. I checked with some examples and multiple commuted sequences and the existing implementation could combine both constants and registers and reduce the number of instructions which was the intention. In doing so however it is not retaining the order of the operands correctly.


================
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]]
----------------
spatel wrote:
> 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.
Right this is possibly due to the existing implementation tryFactorization() which folds the or(and, and) to and(or()). It does it for all scenarios, however likely maintains a fixed position of the operands.


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

https://reviews.llvm.org/D124119