[PATCH] D147472: [VPlan] Use VPLiveOut to update FOR live-out users.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3893
+    LCSSAPhi->addIncoming(ExtractForPhiUsedOutsideLoop, LoopMiddleBlock);
+    State.Plan->removeLiveOut(LCSSAPhi);
   }
----------------
fhahn wrote:
> Ayal wrote:
> > TODO (Independent of this patch): `LiveOuts` is presumably needed only for delayed destruction of VPLiveOuts, as adding an incoming IR Value to LCSSAPhi  should not interfere with traversing the VPUsers of RecurSplice. Why are VPLiveOut VPUsers destroyed here, rather than during the destruction of VPlan, along with all other VPUsers?
> > This applies to the other cases of removeLiveOut() as well.
> At the moment, logic later relies on any live-outs that cannot fix themselves to be removed  so they are excluded from the handling below
> 
> ```
>   // Fix LCSSA phis not already fixed earlier. Extracts may need to be generated
>   // in the exit block, so update the builder.
>   State.Builder.SetInsertPoint(State.CFG.ExitBB->getFirstNonPHI());
>   for (const auto &KV : Plan.getLiveOuts())
>     KV.second->fixPhi(Plan, State);
> ```.
> 
> Once live-outs of FOR & IVs can be handled completely in VPlan, no removal should be necessary.
Thanks for the reminder. LiveOuts that haven't fixed themselves already could alternatively be filtered by checking if their underlying LCSSAPhi has an incoming Value attached; the savings in traversing only those who do comes at the cost of copying ToFix and LiveOuts for delayed destructions, and starting the tear down parts of VPlan early.

This raises some follow-up thoughts - could `fixPhi()` also take care of fixing LCSSAPhi's of FORs where the LiveOut's operand is a FOR splice, instead of taking care of them here? (Ideally all LiveOut phi's should be fixed at the end of VPlan::execute(), but handling reductions is more involved). There may be an issue of reusing same extract for parallel LCSSAPhi's instead of replicating the same extract for each. This is also relevant for fixing other, non-FOR LCSSAPhis, and relates to the general issue of representing extracts in VPlan. It may be simpler to fuse such parallel LCSSAPhi's together, but those currently reside outside the scope of VPlan.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D147472/new/

https://reviews.llvm.org/D147472