[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 a comment.

In D114339#3146382 <https://reviews.llvm.org/D114339#3146382>, @spatel wrote:

> 1. Please commit the baseline tests, so we see diffs here.
> 2. I expect that will show that the commuted variations are not testing what they claim to be testing (you'll need to add instructions to prevent commuting).
> 3. Please add tests with extra uses - since we're creating 2 instructions, the fold should be good as long as any one of the intermediate values can be eliminated (has one use).
>
> Side note: @rampitec recently added a matcher that takes a binary opcode as an input. It probably doesn't make sense here, but it's something to think about if we're trying to make logic folds more regular/predictable ('DeMorganize' folds so we handle patterns where 'and' and 'or' operations are swapped).

Thanks for you suggestion.
I have modified the tests and committed them in D114436 <https://reviews.llvm.org/D114436>; 
I did change the code as well; 
Though it's still conservative in terms that both should have only use. Since, we were removing an instruction from a combination of 3, I thought  if any of them have an extra use then the number of instructions after fold will remains 3.
Also, regarding your last suggestion, I am agree that it's a good idea to make these logic folds more predictable. I will start in this direction.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:3619
+  // (~A | B) ^ A --> ~(A & B) -- There are 4 commuted variants.
+  if (match(&I, m_c_Xor(m_c_Or(m_Not(m_Value(A)), m_Value(B)), m_Deferred(A))))
+    return BinaryOperator::CreateNot(Builder.CreateAnd(A, B));
----------------
foad wrote:
> This probably needs some `m_OneUse`s (but I'm not sure how you decide exactly where to put them).
Thank you!
I added  the `m_OneUse` as per you suggestion  in new commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114339