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

[Adam Nemet via llvm-commits]

anemet added a comment.

Matt, that's a nice example.  Can you please add it as a comment to the code where it's appropriate.


================
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;
+
----------------
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. 

================
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;
+
----------------
How can user of the current value be loop-invariant?

================
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);
+
----------------
We don't add empty /// like that

I actually think that this definition of this should be before isFirstOrderRecurrence.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3558
@@ -3523,1 +3557,3 @@
 
+void InnerLoopVectorizer::vectorizeFirstOrderRecurrence(PHINode *Phi) {
+
----------------
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 :(

================
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");
+
----------------
This is duplicated between here and isFirstOrderRecurrence, would be nice to somehow remove this.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3596-3597
@@ +3595,4 @@
+
+  // We constructed a temporary phi when vectorizing the loop. This phi will
+  // eventually be deleted.
+  auto &PhiParts = getVectorValue(Phi);
----------------
"... when initially vectorizing ..."

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3601-3602
@@ +3600,4 @@
+
+  // Create a phi for the new recurrence. The current value will either be the
+  // splatted initial value or loop-varying vector value.
+  SmallVector<Value *, 8> CurrentParts;
----------------
Outdated comment.

================
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]);
+  }
+
----------------
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?

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3657-3659
@@ +3656,5 @@
+  }
+  Phi->setIncomingValue(Phi->getBasicBlockIndex(LoopScalarPreHeader), Start);
+  Phi->setName("scalar.recur");
+}
+
----------------
A newline before this block?

================
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());
     }
----------------
It's generally not a good practice to mix a multi-line formatting change with a single line (?) functionality change .


http://reviews.llvm.org/D16197