This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Use VPLiveOut to update FOR live-out users.
ClosedPublic

Authored by fhahn on Apr 3 2023, 1:55 PM.

Details

Summary

Instead of iterating over all LCSSA phis in the exit block, collect all
LiveOut users of the FOR splice VPInstruction and only update those
users.

Building on top of D147471, this removes an access to the cost model
after VPlan execution.

Depends on D147471.

Diff Detail

Event Timeline

fhahn created this revision.Apr 3 2023, 1:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2023, 1:55 PM
fhahn requested review of this revision.Apr 3 2023, 1:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2023, 1:55 PM
This revision is now accepted and ready to land.Apr 6 2023, 4:19 PM
fhahn updated this revision to Diff 511658.Apr 7 2023, 4:01 AM

Rebase after recent upstream changes.

Ayal accepted this revision.Apr 8 2023, 10:42 AM

Looks good to me, and raises some thoughts ;-)

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3859

nit (Independent of this patch): else block deserves parentheses considering comments?

3879

nit: "only user should be FirstOrderRecurrenceSplice VPInstruction" >>
"Recurrence Phi must have a single user: FirstOrderRecurrenceSplice"?

3880

nit: worth asserting opcode is FirstOrderRecurrenceSplice ?

3884

(Independent of this patch:) Do multiple live-out users feed multiple LCSSAPhi's, should they all feed the same LCSSAPhi, are there tests covering multiple live-out users?

3887

nit: add message to assert.

3890

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.

fhahn marked 6 inline comments as done.Apr 10 2023, 5:02 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3879

adjusted thanks!

3880

Added the check to the assert.

3884

They feed different LCSSA phis, the test for that is llvm/test/Transforms/LoopVectorize/pr36983.ll.

3887

Thanks, I added a message to the assert.

3890

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.
This revision was automatically updated to reflect the committed changes.
fhahn marked 5 inline comments as done.
Ayal added inline comments.Apr 10 2023, 9:52 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3890

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.