[PATCH] D114339: [InstCombine] simplify (~A | B) ^ A --> ~( A & B)

[Mehrnoosh Heidarpour via Phabricator via llvm-commits]

MehrHeidar marked an inline comment as done.
MehrHeidar added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:3652
+  // (~A | B) ^ A --> ~(A & B)
+  if (match(&I, m_c_Xor(m_c_Or(m_OneUse(m_Not(m_Deferred(Op1))), m_Value(B)), m_Deferred(Op1))) && Op0->hasOneUse())
+    return BinaryOperator::CreateNot(Builder.CreateAnd(Op1, B));
----------------
spatel wrote:
> I'm not sure how the matching logic or one-use logic works here. It seems dangerous to overwrite the Op0/Op1 locals.
> 
> If you want a conservative use check, just put that on the 'or' instruction - as long as we're eliminating that instruction, this transform is a win:
> 
>   // (~A | B) ^ A --> ~(A & B)
>   if (match(Op0, m_OneUse(m_c_Or(m_Not(m_Specific(Op1)), m_Value(B)))))
>     return BinaryOperator::CreateNot(Builder.CreateAnd(Op1, B));
> 
>   // A ^ (~A | B) --> ~(A & B)
>   if (match(Op1, m_OneUse(m_c_Or(m_Not(m_Specific(Op0)), m_Value(B)))))
>     return BinaryOperator::CreateNot(Builder.CreateAnd(Op0, B));
> 
> But I think we're missing a test where the 'not A' has another use. Please add and make sure that looks right. 
Thank you for your suggestions, I was not aware that overwriting the operands might be dangerous. I changed the code.
About the missing test case for an extra use of  `NotA`, I had the test but because previous code was also checking for `NotA` to have only a single use, we were not actually folding this pattern. 
However now, by putting `m_OneUse` on the `or` instruction, folding is done and the changes can be shown against the base result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114339