[PATCH] D47981: [InstCombine] Fold (x >> y) << y -> x & (-1 << y)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In https://reviews.llvm.org/D47981#1127538, @spatel wrote:

> In https://reviews.llvm.org/D47981#1127526, @lebedev.ri wrote:
>
> > Even in light of https://reviews.llvm.org/D46760#1127287, i believe we still can do *this* transform, ...
>
>
> Agreed. I don't think a variable shift amount is a problem, but if it is, then maybe we should enhance SCEV rather than cripple instcombine?


Yep, absolutely, that is why i CC'd Max. That crippling is just wrong.
To point the obvious, it just kinda ignores all the cases that already were shifts.

> (And I think the same could be said for the constant shift amount).





================
Comment at: lib/Transforms/InstCombine/InstCombineShifts.cpp:620-624
     // (X >> C) << C --> X & (-1 << C)
     if (match(Op0, m_Shr(m_Value(X), m_Specific(Op1)))) {
       APInt Mask(APInt::getHighBitsSet(BitWidth, BitWidth - ShAmt));
       return BinaryOperator::CreateAnd(X, ConstantInt::get(Ty, Mask));
     }
----------------
spatel wrote:
> lebedev.ri wrote:
> > ... because we already do it here for when the `y` is constant, regardless of `exact`ness of `shr`.
> > Also, maybe this block should be dropped, now that the more general fold will be done, one not limited to constants?
> Right - I saw that block. I think it's a just a matter of taste because we don't need the one-use check if the shift amount is constant (so you'd have to add an 'if' for that possibility in the later block).
Oh, right, good point. Will keep it then.


Repository:
  rL LLVM

https://reviews.llvm.org/D47981