[PATCH] D61307: [InstCombine] Add new combine to sub folding

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1535-1536
+  // (Y | X) - Y --> (Y | X) ^ Y
+  if (match(Op0, m_OneUse(m_c_Or(m_Value(X), m_Specific(Op1)))))
+    return BinaryOperator::CreateXor(Builder.CreateOr(X, Op1), Op1);
+
----------------
spatel wrote:
> lebedev.ri wrote:
> > cdawson wrote:
> > > lebedev.ri wrote:
> > > > I do not understand. You don't change the inner pattern at all.
> > > > Just do:
> > > > ```
> > > >   if (match(Op0, m_c_Or(m_Specific(Op1), m_Value())))
> > > >     return BinaryOperator::CreateXor(Op0, Op1);
> > > > ```
> > > I totally agree with the change to the return statement and the m_c_Or check. 
> > > 
> > > One point I'd like clarification on is the removal of the m_OneUse. I was including that since the transformation is to facilitate a further (X | Y) ^ Y --> X & ~Y which will only happen if there are no extra uses of (X | Y). If you think we should always transform (X | Y) - Y --> (X | Y) ^ Y I'm happy to remove the m_OneUse.
> > > I was including that since the transformation is to facilitate a further (X | Y) ^ Y --> X & ~Y which will only happen if there are no extra uses of (X | Y).
> > 
> > Because it needs to produce 2 instructions: `~Y` and then `X & (~Y)`,
> > and instcombine is not allowed to increase instruction count,
> > so unless `(X | Y)` was one-use, and would go away,
> > that transform isn't allowed to happen.
> > 
> > So i guess the question is: ignoring any other transforms that may or may not happen afterwards,
> > do we have a preference between `(X | Y) - Y` and `(X | Y) ^ Y`?
> > I'd guess bitop is better regardless of any further optimizations,
> > because LHS is already a bitop.
> > 
> > But if we would prefer `(X | Y) - Y` (then we'd need an inverse transform),
> > then perhaps this should instead be 
> > ```
> >   if (match(Op0, m_OneUse(m_c_Or(m_Specific(Op1), m_Value(X)))))
> >     return BinaryOperator::CreateAnd(X, Builder.CreateNot(Op1));
> > ```
> > directly.
> The xor is probably better for analysis (bit-tracking, demanded-bits, etc), so we want (sub->xor) IMO.
That was my view, too.


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

https://reviews.llvm.org/D61307