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

[Matthew Simpson via Phabricator via llvm-commits]

mssimpso added inline comments.


================
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)
----------------
anna wrote:
> 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. 
> 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?

Yes, if any value in a loop is used externally, LCSSA will create a phi node for it in the exit block. The external uses will then be of the new phi.

> How about these 2 checks under DEBUG:

For counting the external users, yes that would just be checking the LCSSA property, so you don't need to do that.

For the update instruction, you could do something like "assert(!hasOutsideLoopUser(Previous))". See hasOutsideLoopUser for more details.

You could also just go ahead and enable external uses of the update instruction (or do so in another patch if you prefer). The changes here to allow this would be to add "Extract" as the incoming value for the LCSSA phi for "Previous". You'll also need to add the update to AllowedExit over in canVectorizeInstrs.


https://reviews.llvm.org/D31979