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

[bryant via llvm-commits]

bryant added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1960
+    if (X->getType()->isVectorTy())
+      CTy = VectorType::get(CTy, X->getType()->getVectorNumElements());
+    // Derivation for the ult case:
----------------
efriedma wrote:
> Isn't CTy always equal to "X->getType()"?
Since we're morphing `(x << s) ugt c` into `x ugt (c >> s)`, CTy needs to be enough to hold `c >> s`.

`c`'s type would be sufficient thanks to the right shift.

But unlike other binops, shift nodes place no constraint between the types of shiftee and shift amount (I'm really just inferring from my memory of TargetSelectionDAG.td, so I could be wrong).

So X-getType() might not be wide enough to hold `c >> s`.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1970
+                                                : C->lshr(*ShiftAmt);
+    return new ICmpInst(Pred, X, ConstantInt::get(CTy, ShiftC));
+  }
----------------
`ShiftC`. Ugh.


================
Comment at: test/Transforms/InstCombine/icmp-shl-nuw.ll:54-55
+
+define <2 x i1> @icmp_ule_16x2_non_pow2_shift(<2 x i64>) {
+; CHECK-LABEL: @icmp_ule_16x2_non_pow2_shift(
+; CHECK-NEXT:    [[D:%.*]] = icmp ult <2 x i64> %0, <i64 4, i64 4>
----------------
spatel wrote:
> "16x2" --> "12x2" ?
Thanks for catching this.


Repository:
  rL LLVM

https://reviews.llvm.org/D25913