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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri 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:
> 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)
Right. I was wondering about that, but decided to add it after the existing logic. Will move.


================
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))) &&
----------------
lebedev.ri wrote:
> spatel wrote:
> > 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)
> Right. I was wondering about that, but decided to add it after the existing logic. Will move.
Thanks, that is more succinct indeed!
Not too good at `_c_`ommutative matchers yet :/

The original code had full test coverage, and this code does not cause
any changes in tests as compared to my variant, so i *think* it's good.


================
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);
+
----------------
spatel wrote:
> 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.
Yep, sure.
I kinda did not realize that the call `visitAnd()` would already show
this transform, thus it got into this differential.


Repository:
  rL LLVM

https://reviews.llvm.org/D45631