[PATCH] D45631: [InstCombine] Simplify 'xor'/'add' to 'or' if no common bits are set.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Analysis/ValueTracking.cpp:200-201
+    return true;
+  // Try to see if both sides are masked by the same mask (inverted on one side)
+  // I.e.  binop (x & m), (y & ~m)
+  Value *A, *B, *C, *D;
----------------
spatel wrote:
> I think that's logically correct, but it can be coded more simply. All we need to capture is the inverted mask (double-check that this results in the same test changes):
> 
>   // Look for an inverted mask: (X & Y) op (~X & Z).
>   Value *X;
>   if (match(LHS, m_c_And(m_Not(m_Value(X)), m_Value())) &&
>       match(RHS, m_c_And(m_Specific(X), m_Value())))
>     return true;
>   if (match(RHS, m_c_And(m_Not(m_Value(X)), m_Value())) &&
>       match(LHS, m_c_And(m_Specific(X), m_Value())))
>     return true;
> 
It's probably better to put this check above the computeKnownBits calls because this check is cheap (don't waste time doing recursive work if we can match this case first)


================
Comment at: lib/Analysis/ValueTracking.cpp:200-203
+  // Try to see if both sides are masked by the same mask (inverted on one side)
+  // I.e.  binop (x & m), (y & ~m)
+  Value *A, *B, *C, *D;
+  if (match(LHS, m_And(m_Value(A), m_Value(B))) &&
----------------
I think that's logically correct, but it can be coded more simply. All we need to capture is the inverted mask (double-check that this results in the same test changes):

  // Look for an inverted mask: (X & Y) op (~X & Z).
  Value *X;
  if (match(LHS, m_c_And(m_Not(m_Value(X)), m_Value())) &&
      match(RHS, m_c_And(m_Specific(X), m_Value())))
    return true;
  if (match(RHS, m_c_And(m_Not(m_Value(X)), m_Value())) &&
      match(LHS, m_c_And(m_Specific(X), m_Value())))
    return true;



================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:2449-2452
+  // A^B --> A|B iff A and B have no bits set in common.
+  if (haveNoCommonBitsSet(Op0, Op1, DL, &AC, &I, &DT))
+    return BinaryOperator::CreateOr(Op0, Op1);
+
----------------
It's fine to include this in the review, but please make this (and the affected tests) a separate commit. In case there's a problem and/or need to revert, that will make it easier to diagnose the bug.


Repository:
  rL LLVM

https://reviews.llvm.org/D45631