[PATCH] D31979: [LV] Fix the vector code generation for first order recurrence

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added a comment.

In https://reviews.llvm.org/D31979#724852, @anna wrote:

> Hi Matt,
>
> Just FYI: The logic differs slightly from what you suggested in https://reviews.llvm.org/D31910#723721.
>  We need to extract from the correct index of the vectorized phi update. Extracting from index 3 of the vectorized phi (`vector.recur`), will give you the element at `vector.update(3) - VF`, which won't work.


OK, this makes sense to me.



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4094-4095
+  if (VF > 1 && PhiHasUsersOutsideLoop)
+    ExtractForPhiUsedOutsideLoop = Builder.CreateExtractElement(
+        Incoming, Builder.getInt32(VF - 2), "vector.recur.extract.for.phi");
 
----------------
We might as well create this extract unconditionally, regardless of whether the phi is used outside the loop. It would simplify the code here. So you could move it up to where "Extract" is created and not have to worry about setting "PhiHasUsersOutsideLoop". If there are no users, the extract is trivially dead and will be cleaned up by a later pass.

Unlike "Extract", I think we will need to initialize "ExtractForPhiUsedOutsideLoop" to something other than "Incoming" or nullptr. I think we'll need it when we are just unrolling and not vectorizing. Should it be PreviousParts[UF - 2]? See my comment in the test case.

It's probably a good idea to also rename "Extract" to something more descriptive since we now have two of them.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4113-4124
+  // Also, choose the correct value of the vector recurrence for fixing up users of
+  // the recurrence outside the loop.
+  if (PhiHasUsersOutsideLoop)
+    Extract = ExtractForPhiUsedOutsideLoop;
   for (auto &I : *LoopExitBlock) {
     auto *LCSSAPhi = dyn_cast<PHINode>(&I);
     if (!LCSSAPhi)
----------------
This looks a bit strange to me now. The LCSSA phi for "Phi" should always have an incoming value for "ExtractForPhiUsedOutsideLoop" and not "Extract". "Extract" will only be used to initialize the scalar recurrence. I just checked that we don't allow the update instruction to be used externally (I thought we could handle this), so we don't have to worry about that here.


================
Comment at: test/Transforms/LoopVectorize/first-order-recurrence.ll:371
+; UNROLL-NO-IC-CHECK: for.end
+; UNROLL-NO-IC-CHECK: %val.phi.lcssa = phi i32 [ %scalar.recur, %for.body ], [ %vector.recur.extract.for.phi, %middle.block ]
 define i32 @extract_second_last_iteration(i32* %cval, i32 %x)  {
----------------
Can you add a check for the VF = 1, UF > 1 case? I want to make sure the external use is correct when there is no extract.


https://reviews.llvm.org/D31979