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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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));
     }
----------------
bryant wrote:
> 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?
Sorry, I sent that reply too soon. I see now why there is confusion. You're correct (as Alive shows) that eq/ne don't work in general:

http://rise4fun.com/Alive/Yu

What's missing from the equation is the precondition / assertion that the shifted compare constant hasn't dropped any bits:
http://rise4fun.com/Alive/Ee

Known bits analysis should guarantee that cases like that are already folded to true/false, so there is no need for a compare. I'll add asserts to the patch. Makes sense?


https://reviews.llvm.org/D28406