[PATCH] D56355: [InstCombine] Simplify cttz/ctlz + icmp ugt/ult into mask check

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2771-2772
   case Intrinsic::bswap:
+    if (!Cmp.isEquality())
+      return nullptr;
+
----------------
I wonder if it would be less ugly to split this into `foldICmpIntrinsicWithConstant()` and `foldICmpEQIntrinsicWithConstant()`?


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:2789-2791
     // Limit to one use to ensure we don't increase instruction count.
-    unsigned Num = C.getLimitedValue(BitWidth);
-    if (Num != BitWidth && II->hasOneUse()) {
+    if (!II->hasOneUse())
+      return nullptr;
----------------
Can we instead do this check later on in the code, when we know it is needed?


================
Comment at: test/Transforms/InstCombine/cmp-intrinsic.ll:311-401
   %lz = tail call i33 @llvm.cttz.i33(i33 %x, i1 false)
   store i33 %lz, i33* %p
   %cmp = icmp eq i33 %lz, 4
   ret i1 %cmp
 }
 
 define i1 @cttz_ugt_zero_i33(i33 %x) {
----------------
1. Why cram everything into one test file? Why not split into `ctlz.ll` + `cttz.ll` + ...
2. These are `cttz` tests. Using `%lz` is *very* confusing. Please fix this.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56355/new/

https://reviews.llvm.org/D56355