[PATCH] D124183: [InstCombine] Add one use limitation for (X * C2) << C1 --> X * (C2 << C1)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineShifts.cpp:971
     // (C2 << X) << C1 --> (C2 << C1) << X
     if (match(Op0, m_OneUse(m_Shl(m_Constant(C2), m_Value(X)))))
       return BinaryOperator::CreateShl(ConstantExpr::getShl(C2, C1), X);
----------------
bcl5980 wrote:
> bcl5980 wrote:
> > @lebedev.ri @spatel 
> > Based on the rules, can we remove this one-use also ?
> > (C2 << X) << C1 dependent on C2 << X
> > (C2 << C1) << X is independent with C2 << X
> > 
> > One similar case udiv+lshr, do we need add one-use if we add the transform?
> > https://alive2.llvm.org/ce/z/wk6n3q
> add general proof for udiv+lshr:
> https://alive2.llvm.org/ce/z/CfMqxN
Sorry - I lost track of the comments here.

For the 1st question (shift-of-shift), the answer is: yes - we can remove that one-use limitation.
It looks like that transform was added long ago with 16f18ed7b555bce5163f , and there are no tests for multi-use, so we need to add those first to make sure we have coverage.

For the 2nd question (shift-of-udiv), the answer is: yes - we want a one-use limitation on that fold (at least as a first step).
That is because there is very little optimization potential for udiv in IR, and we know that there is a potential regression for codegen (on all targets that I know of) if we replace a shift with a div. If we can show that the backend can remove that div and avoid regressions, then we can consider removing the one-use check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124183