[PATCH] D16197: [LV] Vectorize first-order recurrences

[Matthew Simpson via llvm-commits]

mssimpso added inline comments.

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:532-541
@@ +531,12 @@
+
+  // Get the initial and previous values.
+  auto *Initial = Current->getIncomingValue(0);
+  auto *Previous = Current->getIncomingValue(1);
+
+  // Ensure the initial value is loop-invariant and the previous value is
+  // loop-varying. We already know the current value is loop-varying.
+  if (!TheLoop->isLoopInvariant(Initial))
+    std::swap(Initial, Previous);
+  if (!TheLoop->isLoopInvariant(Initial) || TheLoop->isLoopInvariant(Previous))
+    return false;
+
----------------
anemet wrote:
> Thinking more about this, isn't it true that whether the phi operand is initial or previous depends on their edge assignment rather than whether they loop-variant/invariant. 
When I think more about this, yes, I agree. I'll make the necessary updates. Thanks for catching this.

================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:543-550
@@ +542,10 @@
+
+  // Ensure every user of the current value is loop-varying and dominated by
+  // the previous value. The dominance requirement implies that the loop
+  // vectorizer will not need to vectorize the initial value outside the loop.
+  for (User *U : Current->users())
+    if (auto *I = dyn_cast<Instruction>(U))
+      if (TheLoop->isLoopInvariant(I) ||
+          !DT->dominates(cast<Instruction>(Previous), I))
+        return false;
+
----------------
anemet wrote:
> How can user of the current value be loop-invariant?
Current (the Phi) could be used outside the loop. Reductions are an example where a Phi is used externally. I actually don't think we need this restriction though, because we can handle this case in code generation. The use would just be of the last value of the recurrence, which we already extract in middle.block.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:368-381
@@ +367,16 @@
+  /// Vectorize first-order recurrences.
+  ///
+  /// A first-order recurrence is a non-reduction recurrence relation in which
+  /// the value of the recurrence in the current loop iteration equals a value
+  /// defined in the previous iteration. For example, in the loop below:
+  ///
+  ///   for (int i = 0; i < n; ++i)
+  ///     b[i] = a[i] - a[i - 1];
+  ///
+  /// There is a first-order recurrence on "a". The recurrence is a PHI node of
+  /// the form:
+  ///
+  ///   recurrence = phi [a[-1], loop.preheader], [a[i], loop.header]
+  ///
+  void vectorizeFirstOrderRecurrence(PHINode *Phi);
+
----------------
anemet wrote:
> We don't add empty /// like that
> 
> I actually think that this definition of this should be before isFirstOrderRecurrence.
Sure, I'll move it.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3558
@@ -3523,1 +3557,3 @@
 
+void InnerLoopVectorizer::vectorizeFirstOrderRecurrence(PHINode *Phi) {
+
----------------
anemet wrote:
> Sorry but I forgot that "fix" is an actual "phase" here.  We first do the default widening then we fix up the phis.  Can you please rename it back, sorry :(
No problem!

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3573-3585
@@ +3572,15 @@
+
+  // Get the initial and previous values of the scalar recurrence. First-order
+  // recurrences are of the form "current = phi(initial, previous)".
+  auto *ScalarInit = Phi->getIncomingValue(0);
+  auto *Previous = Phi->getIncomingValue(1);
+  if (!OrigLoop->isLoopInvariant(ScalarInit))
+    std::swap(ScalarInit, Previous);
+
+  // Ensure the previous value is loop-varying and the initial value is
+  // loop-invariant.
+  assert(!OrigLoop->isLoopInvariant(Previous) &&
+         "Previous value is loop-varying");
+  assert(OrigLoop->isLoopInvariant(ScalarInit) &&
+         "Initial value isn't loop-invariant");
+
----------------
anemet wrote:
> This is duplicated between here and isFirstOrderRecurrence, would be nice to somehow remove this.
Sure, I will delete these assertions here.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3625-3636
@@ +3624,14 @@
+
+  // Shuffle the current and previous vector and update the vector parts.
+  for (unsigned Part = 0; Part < UF; ++Part) {
+    auto *Shuffle = VF > 1
+                        ? Builder.CreateShuffleVector(
+                              CurrentParts[Part], PreviousParts[Part],
+                              ConstantVector::get(ShuffleMask))
+                        : CurrentParts[Part];
+    PhiParts[Part]->replaceAllUsesWith(Shuffle);
+    cast<Instruction>(PhiParts[Part])->eraseFromParent();
+    PhiParts[Part] = Shuffle;
+    CurrentParts.push_back(PreviousParts[Part]);
+  }
+
----------------
anemet wrote:
> I think that CurrentParts should be a single value, this is the value that's incoming into the iteration (whether the real loop iteration or an unrolled loop iteration).   We may also want to use the name VecPhi rather than "current" and add comment clarifying the above.
> 
> What do you think?
Yes, I don't think we need to keep track of CurrentParts as a SmallVector since a single value should do. Changing the name is probably a good idea to avoid confusion.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3735-3742
@@ -3597,10 +3734,10 @@
   PHINode* P = cast<PHINode>(PN);
-  // Handle reduction variables:
-  if (Legal->isReductionVariable(P)) {
-    for (unsigned part = 0; part < UF; ++part) {
-      // This is phase one of vectorizing PHIs.
-      Type *VecTy = (VF == 1) ? PN->getType() :
-      VectorType::get(PN->getType(), VF);
-      Entry[part] = PHINode::Create(
-          VecTy, 2, "vec.phi", &*LoopVectorBody.back()->getFirstInsertionPt());
+
+  // Handle recurrences. We create a temporary phi in the vector type that we
+  // will fix later.
+  if (Legal->isReductionVariable(P) || Legal->isFirstOrderRecurrence(P)) {
+    for (unsigned Part = 0; Part < UF; ++Part) {
+      auto *Ty = (VF == 1) ? PN->getType() : VectorType::get(PN->getType(), VF);
+      Entry[Part] = PHINode::Create(
+          Ty, 2, "vec.phi", &*LoopVectorBody.back()->getFirstInsertionPt());
     }
----------------
anemet wrote:
> It's generally not a good practice to mix a multi-line formatting change with a single line (?) functionality change .
Sure, we can fix the formatting in a separate patch.


http://reviews.llvm.org/D16197