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

[Amaury SECHET via llvm-commits]

deadalnix added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:1414-1452
@@ -1399,37 +1413,41 @@
     break;
   case Intrinsic::cttz: {
     // If all bits below the first known one are known zero,
     // this value is constant.
-    IntegerType *IT = dyn_cast<IntegerType>(II->getArgOperand(0)->getType());
+    Value *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);
+    computeKnownBits(Op0, KnownZero, KnownOne, 0, II);
     unsigned TrailingZeros = KnownOne.countTrailingZeros();
     APInt Mask(APInt::getLowBitsSet(BitWidth, TrailingZeros));
     if ((Mask & KnownZero) == Mask)
       return replaceInstUsesWith(CI, ConstantInt::get(IT,
                                  APInt(BitWidth, TrailingZeros)));
-
+    if (KnownOne != 0 || isKnownNonZero(Op0, DL))
+      makeCttzCtlzZeroUndef(*this, II);
     }
     break;
   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());
+    Value *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);
+    computeKnownBits(Op0, 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))
+      makeCttzCtlzZeroUndef(*this, II);
     }
     break;
----------------
spatel wrote:
> I was hoping that you could refactor this whole thing, so there's no code duplication. 
> 
> On 2nd look, I see that the existing fold is actually in the wrong place. Since we're able to fold to a constant, I think it belongs in InstSimplify. Can you fix that or add a 'FIXME' comment?
> 
> I'd still prefer that you refactor this ahead of the functional change. If my first comment about makeCttzCtlzZeroUndef() being one line of code is correct, then it's still just one helper function for this whole thing.
> 
> Let me know if I'm not seeing this correctly.
The bitmask manipulation previous this, while following a similar pattern, are different for cttz and ctlz.

This is NOT constant folding in any way. It is making cttz and/or ctlz undefined for 0 when possible as it lead to better codegen, at least on X86. No the function cannot be simplified to a one liner.


https://reviews.llvm.org/D23134