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

[Matthew Simpson via llvm-commits]

mssimpso added a comment.

Hi Adam,

Thanks very much for the feedback. Yes, the initial value doesn't have to be broadcast to all lanes since just an insert will do. I will upload a new version with your suggestions. Let me walk through the simple example I mentioned in the summary. For this loop, the (shorthand) scalar IR looks something like this:

  scalar.ph:
    s_init = a[-1]
    br scalar.body
  
  scalar.body:
    s1 = phi [s_init, scalar.ph], [s2, scalar.body]
    i = phi [0, scalar.ph], [i+1, scalar.body]
    s2 = a[i]
    b[i] = s2 - s1
    br cond, scalar.body, ...

Here, s1 is a non-reduction recurrence that we currently give up on. This patch calls it a first-order recurrence (because it's value depends on the previous iteration) and we try to vectorize it. The check in isFirstOrderRecurrence basically ensures that s_init is loop-invariant, that s2 is in the loop header (and thus loop-varying), and that every use of s1 is dominated by the definition of s2 (see below). The vectorized IR looks something like this for VF=4:

  vector.ph:
    v_init = vector(..., ..., ..., a[-1])
    br vector.body
  
  vector.body
    v1 = phi [v_init, vector.ph], [v2, vector.body]
    i = phi [0, vector.ph], [i+4, vector.body]
    v2 = a[i, i+1, i+2, i+3];
    v3 = vector(v1(3), v2(0, 1, 2))
    b[i, i+1, i+2, i+3] = v2 - v3
    br cond, vector.body, middle.block
  
  middle.block:
    x = v2(3)
    br scalar.ph
  
  scalar.ph:
    s_init = phi [x, middle.block], [a[-1], otherwise]
    br scalar.body

The dominance requirement is there so that we can shuffle v1 with v2 before this value is needed for the assignment to b. After we leave the vector loop, we extract the next value of the recurrence (x) to use as the initial value when jumping to the scalar portion. I hope that helps.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:367
@@ -366,1 +366,3 @@
 
+  /// Vectorize first-order recurrences.
+  void fixFirstOrderRecurrence(PHINode *Phi);
----------------
anemet wrote:
> I guess a definition of first-order recurrence should be added here.
Agreed.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3323-3324
@@ +3322,4 @@
+    //
+    // FIXME: We need to refactor this function since the recurrences in
+    //        RdxPHIsToFix are not necessarily reductions.
+    if (Legal->isFirstOrderRecurrence(RdxPhi)) {
----------------
anemet wrote:
> I actually think we need to this before this patch ;).  This is a 200-line loop and we shouldn't pile on more strangeness.
> 
> I think that all we need to do is to rename RdxPHIsToFix to PHIsToFix in a prequel NFC patch.
Sure, I'll push a patch that renames RdxPHIsToFix to PHIsToFix first.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3326
@@ +3325,3 @@
+    if (Legal->isFirstOrderRecurrence(RdxPhi)) {
+      fixFirstOrderRecurrence(RdxPhi);
+      continue;
----------------
anemet wrote:
> I know that we use fix... in other places but that's pretty general.  How about vectorize... or something like that.
vectorizeFirstOrderRecurrence sounds good to me.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3564-3567
@@ +3563,6 @@
+  // recurrences are of the form "current = phi(initial, previous)".
+  auto *Initial = Phi->getIncomingValue(0);
+  auto *Previous = Phi->getIncomingValue(1);
+  if (!OrigLoop->isLoopInvariant(Initial))
+    std::swap(Initial, Previous);
+
----------------
anemet wrote:
> assert that Previous is loop-invariant at this point?
Good idea.


http://reviews.llvm.org/D16197