[PATCH] D23134: Make cltz and cttz zero undef when the operand cannot be zero in InstCombine

[Sanjay Patel via llvm-commits]

spatel added a comment.

Just curious, is there a fold or codegen that doesn't work because of the undef?


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1403
@@ -1402,2 +1402,3 @@
     // this value is constant.
-    IntegerType *IT = dyn_cast<IntegerType>(II->getArgOperand(0)->getType());
+    auto Op0 = II->getArgOperand(0);
+    IntegerType *IT = dyn_cast<IntegerType>(Op0->getType());
----------------
I'm not sure where we draw the line on 'auto', but I think the general preference is to use the actual type (Value *) in cases like this. At the least, this should be 'auto *' according to the coding standards.

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1410
@@ -1408,3 +1409,3 @@
     APInt KnownOne(BitWidth, 0);
     computeKnownBits(II->getArgOperand(0), KnownZero, KnownOne, 0, II);
     unsigned TrailingZeros = KnownOne.countTrailingZeros();
----------------
Use 'Op0' since we have it now.

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1416-1417
@@ -1414,3 +1415,4 @@
                                  APInt(BitWidth, TrailingZeros)));
-
+    if (KnownOne != 0 || isKnownNonZero(Op0, DL))
+      goto MakeCttzCltzUndef;
     }
----------------
I don't see anything about 'goto' in the coding standard doc, but I was trained to feel nervous anytime I see one. Make this a static helper or lambda instead?

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1421-1438
@@ -1418,18 +1420,20 @@
   case Intrinsic::ctlz: {
     // If all bits above the first known one are known zero,
     // this value is constant.
-    IntegerType *IT = dyn_cast<IntegerType>(II->getArgOperand(0)->getType());
+    auto Op0 = II->getArgOperand(0);
+    IntegerType *IT = dyn_cast<IntegerType>(Op0->getType());
     // FIXME: Try to simplify vectors of integers.
     if (!IT) break;
     uint32_t BitWidth = IT->getBitWidth();
     APInt KnownZero(BitWidth, 0);
     APInt KnownOne(BitWidth, 0);
     computeKnownBits(II->getArgOperand(0), KnownZero, KnownOne, 0, II);
     unsigned LeadingZeros = KnownOne.countLeadingZeros();
     APInt Mask(APInt::getHighBitsSet(BitWidth, LeadingZeros));
     if ((Mask & KnownZero) == Mask)
       return replaceInstUsesWith(CI, ConstantInt::get(IT,
                                  APInt(BitWidth, LeadingZeros)));
-
+    if (KnownOne != 0 || isKnownNonZero(Op0, DL))
+      goto MakeCttzCltzUndef;
     }
     break;
----------------
How about refactoring this whole case into a helper function since it's nearly identical to the cttz case? That would remove the question about the 'goto' too.

================
Comment at: test/Transforms/InstCombine/intrinsics.ll:383
@@ +382,3 @@
+define i32 @ctlz_make_undef(i32 %a) {
+entry:
+  %or = or i32 %a, 8
----------------
You can remove the label to reuce the test and the CHECKs.


https://reviews.llvm.org/D23134