[PATCH] D18777: [ValueTracking] An improvement to IR ValueTracking on Non-negative Integers

[Li Huang via llvm-commits]

lihuang added a comment.

I will separate the change in IndVarSimplify into another patch and create a new diff.


================
Comment at: lib/Analysis/ValueTracking.cpp:805
@@ +804,3 @@
+// known bits of its first operand.
+static void computeKnownSignBitFromShift(Operator *I, 
+                                         APInt &KnownZero, APInt &KnownOne,
----------------
sanjoy wrote:
> Can this rule be split out into a separate change of its own (with its own tests etc.)?
Yes it could, I will make a separate patch for this with its own tests.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1309
@@ -1308,1 +1308,3 @@
+  if ((IsSigned ? isa<SExtInst>(DU.NarrowUse) : isa<ZExtInst>(DU.NarrowUse)) ||
+      (isa<ZExtInst>(DU.NarrowUse) && DU.NeverNegative)) { 
     Value *NewDef = DU.WideDef;
----------------
sanjoy wrote:
> Usually we refrain from changing multiple passes at once.  Can you please move these IndVarSimplify changes to their own patch, with their own tests?
Sure, I will make a separate patch for this change and its tests.

================
Comment at: test/Analysis/ValueTracking/shift-nsw.ll:11
@@ +10,3 @@
+
+for.body.11:                                      ; preds = %for.body.11, %for.preheader
+  %j.0320 = phi i32 [ 0, %for.preheader ], [ %inc, %for.body.11 ]
----------------
sanjoy wrote:
> - The filename is misleading -- there is a lot going on here other than the "nsw shift" optimization.
> 
> - Please break these into small bits of IR functions each of which show one transform.
> 
> - Use `CHECK-LABEL`
Thanks! I will update this test.


http://reviews.llvm.org/D18777