[PATCH] D88429: [InstCombine] visitTrunc - trunc (*shr (trunc A), C) --> trunc(*shr A, C)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

LGTM, but please see https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20200928/835878.html



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:859
+    unsigned AWidth = AType->getScalarSizeInBits();
+    unsigned MaxShiftAmt = std::min<unsigned>(DestWidth, AWidth - DestWidth);
+
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > RKSimon wrote:
> > > lebedev.ri wrote:
> > > > I do not understand why this isn't just `unsigned MaxShiftAmt = AWidth - DestWidth;`
> > > I tried that and hit issues with cases like this: https://alive2.llvm.org/ce/z/qV29xP truncating i64 -> i9 -> i8 - so I think there's a typo and it should be std::min<unsigned>(DestWidth, SrcWidth - DestWidth)
> > It is correct that  https://alive2.llvm.org/ce/z/qV29x9 is a micompile, it should not be getting folded:
> > https://rise4fun.com/Alive/RTe7 
> > 
> > However to me it still looks like that the precondition i proposed is correct:
> > https://rise4fun.com/Alive/saBR
> > 
> Err, i guess i'm just not reading what i'm writing.
> Of course `unsigned MaxShiftAmt = AWidth - DestWidth;` won't work, because i'm prooving that `C1 u<= width(%b)-width(%R)`.
> So it should be `unsigned MaxShiftAmt = C0->getType()->getScalarSizeInBits() - DestWidth;`
> :/
> 
Please do mark done comments as such


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88429