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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineShifts.cpp:732-735
         // (X << C1) >>u C2  --> (X >>u (C2 - C1)) & (-1 >> C2)
         Value *NewLShr = Builder.CreateLShr(X, ShiftDiff, "", I.isExact());
         APInt Mask(APInt::getLowBitsSet(BitWidth, BitWidth - ShAmt));
         return BinaryOperator::CreateAnd(NewLShr, ConstantInt::get(Ty, Mask));
----------------
lebedev.ri wrote:
> spatel wrote:
> > lebedev.ri wrote:
> > > spatel wrote:
> > > > As with D47981, we could consolidate this, but the constant version doesn't need the one-use check. 
> > > > 
> > > > I think it's fine either way, but let's keep both patches consistent in their structure.
> > > > As with D47981, we could consolidate this, but the constant version doesn't need the one-use check.
> > > I haven't really though about that..
> > > Sounds like i should add multi-use tests with constants to the tests.
> > Oops - misplaced this inline comment. It was meant for the block where ShlAmt == ShAmt.
> > As with D47981, we could consolidate this, but the constant version doesn't need the one-use check.
> 
> Hmm, this is actually broken. That is only true if either the shifts are the same constant,
> or `shl` is `nuw`, else it produces one extra instruction.
> 
I will add tests, but there is one test in `test/Transforms/InstCombine/shift.ll` that breaks
if i 'fix' this, so maybe this is intentional..


Repository:
  rL LLVM

https://reviews.llvm.org/D47980