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

[Anna Thomas via Phabricator via llvm-commits]

anna planned changes to this revision.
anna added a comment.

Need change based on Matt's comment to handle code gen when just unrolling and not vectorizing.



================
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");
 
----------------
mssimpso wrote:
> 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.
Agreed, will make the `ExtractForPhiUsedOutsideLoop` unconditional, and allow dce to eliminate it. I was under the impression these extracts would contribute to the cost model, but looking at the code, we only consider the instructions within the loop. 

I'll take a look at the UF>1 and VF = 1 case. Thanks!


================
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)
----------------
mssimpso wrote:
> 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.
Ah! We can always use `ExtractForPhiUsedOutsideLoop` as the incoming value, since the loop is in LCSSA form. So, this `Phi` will be used only in LCSSA phis outside the loop, and it's occurrence will be only at the loop exit block. 
Is that right? 

How about these 2 checks under `DEBUG`:
1. Use the `PhiHasUsersOutsideLoop` to count number of users. It should be equal to one when we hit `LCSSAPhi->getIncomingValue(0) == Phi`. Or is this just checking LCSSA property? :)
2. update instruction has no external uses. If that's false, then we would get incorrect code gen here. 


https://reviews.llvm.org/D31979