[PATCH] D12706: Handle non-constant shifts in computeKnownBits, and use computeKnownBits for constant folding in InstCombine/Simplify

[Philip Reames via llvm-commits]

reames added a comment.

Drive by review.


================
Comment at: lib/Analysis/InstructionSimplify.cpp:4062
@@ +4061,3 @@
+  // value even when the operands are not all constants.
+  if (!Result && I->getType()->isIntegerTy()) {
+    unsigned BitWidth = I->getType()->getScalarSizeInBits();
----------------
As a future enhancement, extending this to vectors and floating point types would make sense.  A future patch is fine.

================
Comment at: lib/Analysis/ValueTracking.cpp:966
@@ +965,3 @@
+template <typename KZFunctor, typename KOFunctor>
+static void computeKnownBitsFromShiftOperator(Operator *I,
+              APInt &KnownZero, APInt &KnownOne,
----------------
Reading the code, it's really not clear what KnownX vs KnownX2 mean.  Are there better names which could be used here?  Same with KZF and KOF.  At minimum, there's some documentation needed here.

================
Comment at: lib/Analysis/ValueTracking.cpp:1121
@@ -1120,3 +1186,1 @@
     // (ashr X, C1) & C2 == 0   iff  (-1 >> C1) & C2 == 0
-    if (ConstantInt *SA = dyn_cast<ConstantInt>(I->getOperand(1))) {
-      // Compute the new bits that are at the top now.
----------------
It's not immediately clear to me that the new code implements all of the cases the old code did.  It would have been much easier to review if you'd first refactored out the helper function with the existing code, then added the new functionality.  Not asking you to do that now, but doing that way would have made review easier.  

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:2733
@@ -2732,1 +2732,3 @@
 
+    // In general, it is possible for computeKnownBits to determine all bits in a
+    // value even when the operands are not all constants.
----------------
Doesn't instcombine call SimplifyInstruction internally?  If so, why do we need to duplicate this block of code here?


http://reviews.llvm.org/D12706