[PATCH] D21620: [LV] Don't widen trivial induction variables

[Matthew Simpson via llvm-commits]

mssimpso added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:316
@@ -314,2 +315,3 @@
     MinBWs = &MinimumBitWidths;
+    ValuesNotWidened = &VecValuesToIgnore;
     Legal = L;
----------------
anemet wrote:
> Since you're holding on to this in a member, it's less surprising if the reference is live throughout the lifetime of the InnerLoopVectorizer.  I think that we should pass this in the ctor.  I see no particular reason not to do this way.
Would it make sense to just pass the Cost Model in the ctor? MinimumBitWidths comes from there as well. Actually, I'm not sure why we don't pass the Legality in the ctor either.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4219-4233
@@ -4164,7 +4218,17 @@
       }
+
+      // If an induction variable is only used for counting loop iterations or
+      // calculating addresses, it shouldn't be widened. Scalarize the step
+      // vector to give InstCombine a better chance of simplifying it.
+      if (VF > 1 && ValuesNotWidened->count(P)) {
+        for (unsigned part = 0; part < UF; ++part)
+          Entry[part] = getScalarizedStepVector(V, VF * part, II.getStep(), VF);
+        return;
+      }
+
       Value *Broadcasted = getBroadcastInstrs(V);
       // After broadcasting the induction variable we need to make the vector
       // consecutive by adding 0, 1, 2, etc.
       for (unsigned part = 0; part < UF; ++part)
         Entry[part] = getStepVector(Broadcasted, VF * part, II.getStep());
     } else {
----------------
anemet wrote:
> Looks like this and the code below under trunc are quite similar.  Can we factor this is out to a helper in a prequel to this patch and then add the special case in the to the helper?
I think so - I'll give it a shot and post for review.

================
Comment at: test/Transforms/LoopVectorize/induction.ll:70-80
@@ -68,1 +69,13 @@
 
+; Make sure we don't widen induction variables that don't have interesting
+; users inside the loop. Doing so causes us to create unnecessary phi nodes and
+; computation inside the loop.
+;
+; for (int i = 0; i < n; ++i)
+;   sum += a[i];
+;
+; IND-LABEL:    @scalarize_induction_variable(
+; IND:          vector.body:
+; IND:            %index = phi i64 [ 0, %vector.ph ], [ %index.next, %vector.body ]
+; IND-NOT:        add i64 {{.*}}, 2
+; IND:            getelementptr inbounds i64, i64* %a, i64 %index
----------------
anemet wrote:
> I think that the testcase from the PR I mentioned would better highlight the problem of vectorizing the induction variable.  I.e. in this case we don't actually end up with a vector induction variable but a secondary unnecessary IV.
I'll add that test case as well.


http://reviews.llvm.org/D21620