[PATCH] D109807: [InstCombine] Narrow type of logical operation chains in certain cases

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D109807#3011145 <https://reviews.llvm.org/D109807#3011145>, @mnadeem wrote:

> I shouldn't have given this revision a general title, I was only targeting a specific case i.e. `extend(X) | (extend(Y) | Z)`

No problem - I was just wondering if you had a more general solution in mind. We handle some of the most basic reassociation patterns in instcombine already, so this patch seems fine to me.

>   if (match(Op0, m_OneUse(m_c_BinOp(m_CombineAnd(m_ZExtOrSExt(m_Value()), m_Value(A)), m_Value(B)))) &&
>       match(Op1, m_OneUse(m_c_BinOp(m_CombineAnd(m_ZExtOrSExt(m_Value()), m_Value(C)), m_Value(D))))) {

On the other hand, this is probably going too far. We can keep adding logic ops to the chain to move the casts further and further apart, and there's no realistic way to bring them back together in instcombine. That's the job of the -reassociate pass.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1613
   CastInst *Cast0 = dyn_cast<CastInst>(Op0);
-  if (!Cast0)
-    return nullptr;
+  if (!Cast0) {
+    std::swap(Op0, Op1);
----------------
It's not clear to me why this swap is necessary. Do you have a test case where a logic binop has a cast as operand 0 and a binop as operand 1? Complexity-based canonicalization is supposed to prevent that. See InstCombinerImpl::SimplifyAssociativeOrCommutative().


================
Comment at: llvm/test/Transforms/InstCombine/and-xor-or.ll:474
 
 ; Negative test with more uses.
 define i64 @sext_or_chain_two_uses1(i64 %a, i16 %b, i16 %c, i64 %d) {
----------------
I think we're missing tests:
1. Negative test with different logic opcodes.
2. Negative test with different cast opcodes.
3. Test with different cast source types.
4. Test with multiple uses of cast instruction(s).
5. Tests where the first cast is operand 1 of the logic op (notice in the original tests that the operands are commuted from where they started - search around the test directory for "thwart complexity-based canonicalization" for ways to prevent that).


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

https://reviews.llvm.org/D109807