[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:
> `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.



https://reviews.llvm.org/D28406