[PATCH] D25913: [InstCombine] Fold nuw left-shifts in `ugt`/`ule` comparisons.

[Sanjay Patel via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1955
+  // comparison only really happens in the pre-shifted bits. This also holds for
+  // <=u and >u, but the latter two are canonicalized into the former.
+  if (Shl->hasNoUnsignedWrap()) {
----------------
">u" should be ">=u"


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1961
+
+    if (C->ugt(0) && Pred == ICmpInst::ICMP_ULT)
+      return new ICmpInst(ICmpInst::ICMP_ULE, X,
----------------
I don't think you need to check that the constant is ugt(0) here because:
icmp ult %x, 0
must always be simplified to 'false' ahead of this.
Let me know if I've misunderstood that check.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1962-1963
+    if (C->ugt(0) && Pred == ICmpInst::ICMP_ULT)
+      return new ICmpInst(ICmpInst::ICMP_ULE, X,
+                          ConstantInt::get(CTy, (*C - 1).lshr(*ShiftAmt)));
+    else if (Pred == ICmpInst::ICMP_UGT)
----------------
bryant wrote:
> bryant wrote:
> > spatel wrote:
> > > Why return a ULE icmp here rather than ULT?
> > Are you suggesting to emit `x u< [(c - 1) >> s] - 1`? That just seems messier.
> `+ 1`, not `- 1`.
If you don't change the predicate, I think it makes the code easier to read (as well as short-circuiting the subsequent canonicalization to ULT).

Could look more like this?

```
  if (Shl->hasNoUnsignedWrap() &&
      (Pred == ICmpInst::ICMP_UGT || Pred == ICmpInst::ICMP_ULT)) {
    Type *CTy = IntegerType::get(Cmp.getContext(), C->getBitWidth());
    if (X->getType()->isVectorTy())
      CTy = VectorType::get(CTy, X->getType()->getVectorNumElements());

    APInt ShiftedC = Pred == ICmpInst::ICMP_ULT ? (*C - 1).lshr(*ShiftAmt) + 1
                                                : C->lshr(*ShiftAmt);
    return new ICmpInst(Pred, X, ConstantInt::get(CTy, ShiftedC));
  }

```


================
Comment at: test/Transforms/InstCombine/icmp-shl-zext-simplify.ll:39-42
   %c = shl nuw <2 x i64> %0, <i64 16, i64 16>
   %d = icmp ule <2 x i64> %c, <i64 65535, i64 65535>
   ret <2 x i1> %d
 }
----------------
Please change the constant in this test so we have more test coverage to show the correct non-zero constant for a ule/ult comparison. Eg, 196608 (0x30000) --> 4 ? 


Repository:
  rL LLVM

https://reviews.llvm.org/D25913