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

[Sanjay Patel via llvm-commits]

spatel added a comment.

The patch will crash for vectors:
define <2 x i1> @icmp_ugt_16_vec(<2 x i64> %x) {

  %c = shl nuw <2 x i64> %x, <i64 16, i64 16>
  %d = icmp ugt <2 x i64> %c, <i64 65535, i64 65535>
  ret <2 x i1> %d

}

Splat vectors should be handled the same as scalars. You can look at the fold just above yours for a model of how to fix it.



================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:1969-1970
 
+  // when the shift is nuw and pred is >u or <=, then comparison only really
+  // happens in the pre-shifted bits.
+  if (Shl->hasNoUnsignedWrap() &&
----------------
Nit: start sentence with capital letter.


================
Comment at: test/Transforms/InstCombine/icmp-shl-zext-simplify.ll:3-13
+define i32 @icmp_ugt_16(i16) {
+; CHECK-LABEL: @icmp_ugt_16
+; CHECK-NEXT: icmp ne i16 %0, 0
+; CHECK-NEXT: zext
+; CHECK-NEXT: ret
+  %b = zext i16 %0 to i64
+  %c = shl i64 %b, 16
----------------
1. You can simplify the tests in all cases by:
a. Remove the leading zext
b. Add 'nuw' to the shl
c. Remove the trailing zext (just return the i1)

2. Please use the python script at utils/update_test_checks.py to auto-generate the check lines.

3. I'd prefer to see tests that check different constant values rather than different type sizes. Ie, we can be pretty sure that InstCombine folds work for arbitrary sizes, so those tests don't add a lot of value, but we need to make sure that the new constant is created correctly for the new fold:

```
  define i1 @icmp_ugt_16trunc(i64 %x) {
    %shl = shl nuw i64 %x, 16
    %cmp = icmp ugt i64 %shl, 65537
    ret i1 %cmp
  }

```


Repository:
  rL LLVM

https://reviews.llvm.org/D25913