[PATCH] D86363: InstCombine transform pattern "(A ^ B) | ~(A | B) -> ~(A & B)" added

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2703
   // (A ^ B) | ((B ^ C) ^ A) -> (A ^ B) | C
   if (match(Op0, m_Xor(m_Value(A), m_Value(B))))
     if (match(Op1, m_Xor(m_Xor(m_Specific(B), m_Value(C)), m_Specific(A))))
----------------
xbolva00 wrote:
> Does this work for commuted case? @spatel
Off-topic for this review, but no it doesn't. 
If I'm seeing that correctly, there are 16 potential commuted variants, and we only handle 2.
Looks like the motivating cases had a constant operand, so the more general patterns weren't noticed even though the matchers were generalized:
rG42af360

The 'and' variant of this pattern is also missing commutes. We could sprinkle around more "m_c_Xor" and probably get these, but it's not something instcombine can handle in the general case - it's a job for reassociation + CSE, and they seem to work as expected here. So I think the best thing is to add some code comments and/or restrict these to matching a pattern with a constant. Also, add some PhaseOrdering tests for all of those commute tests to make sure they are not escaping through "-O1".


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

https://reviews.llvm.org/D86363