[PATCH] D28406: [InstCombine] icmp sgt (shl nsw X, C1), C0 --> icmp sgt X, C0 >> C1

[bryant via Phabricator via llvm-commits]

bryant added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1945
       APInt ShiftedC = C->lshr(*ShiftAmt);
       return new ICmpInst(Pred, X, ConstantInt::get(ShType, ShiftedC));
     }
----------------
spatel wrote:
> bryant wrote:
> > `Cmp.isEquality() || Pred == ICmpInst::ICMP_UGT` looks equivalent to `Pred == ICMP_UGE`. From memory, the unsigned counterparts only work with ugt and ule. It doesn't work with uge.
> > 
> > ```
> > Name: ugt
> > %a = shl nsw i8 %x, C1
> > %b = icmp ugt %a, C0
> >   =>
> > %b = icmp ugt %x, (C0 >> C1)
> > 
> > ----------------------------------------
> > Optimization: ugt
> > Done: 1
> > Optimization is correct! 
> > 
> > Name: eq
> > %a = shl nsw i8 %x, C1
> > %b = icmp eq %a, C0
> >   =>
> > %b = icmp eq %x, (C0 >> C1)
> > 
> > ----------------------------------------
> > Optimization: eq
> > ERROR: Mismatch in values of i1 %b
> > ```
> > 
> > http://rise4fun.com/Alive/D1
> Except that Cmp.isEquality() has nothing to do with ICMP_UGE:
>   static bool isEquality(Predicate P) {
>     return P == ICMP_EQ || P == ICMP_NE;
>   }
> 
> I'll take this as a suggestion to improve readability by spelling out the relevant predicates instead of using isEquality, but there is no functional change.
> 
It doesn't work with uge because it doesn't work with eq. Line 1942 would allow the transform to happen if Pred == ICmpInst::ICMP_EQ, right? Am I mis-reading?


https://reviews.llvm.org/D28406