This is an archive of the discontinued LLVM Phabricator instance.

[LV] Remove unneeded createHeaderBranch.(NFCI)
ClosedPublic

Authored by fhahn on Mar 14 2022, 10:18 AM.

Details

Summary

The only remaining use was to get the exit block of the loop. Instead of
relying on the loop, use the successor of VectorHeaderBB
(LoopMiddleBlock) directly to set VPTransformState::CFG::ExitB

Depends on D121621.

Diff Detail

Event Timeline

fhahn created this revision.Mar 14 2022, 10:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 10:18 AM
fhahn requested review of this revision.Mar 14 2022, 10:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 10:18 AM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added inline comments.Mar 27 2022, 9:16 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
634

It still also allocates a loop object for the new vector loop, placing the header in it, and could return it, but doing so would be useless - the loop is retrieved when needed from its header block.

7590

Would be good to set here the minimal fields of State needed, PrevBB/VectorLoopPreHeader being an anchor, and complement other caching fields later - possibly LastBB included?

llvm/lib/Transforms/Vectorize/VPlan.cpp
914

Is it no longer possible to retrieve LoopMiddleBlock as the exit block of L, at this time? (Intentionally, from the summary: "Instead of relying on the loop ...")
Alternatively, retrieve LastBB/LoopMiddleBlock as VectorHeaderBB->getSingleSuccessor(); ?

fhahn updated this revision to Diff 418826.Mar 29 2022, 2:53 AM
fhahn marked an inline comment as done.

Add back loop creation part to the comment, move all initial CFGTransformState initialization to single place as suggested, thanks!

fhahn marked an inline comment as done.Mar 29 2022, 2:54 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
634

Ah yes, added back the bit about allocating the loop, thanks!

7590

I tried to move all revenant initializations here.

Ayal added inline comments.Mar 29 2022, 12:35 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7590

Oh, no, sorry ... I meant LVP should (continue to) set the minimal amount of fields in State here, and let VPlan complement whatever additional fields it needs later, as done now. Wanted to make sure if ExitBB needs to be set to middleBlock here, instead of setting it later to L->getExitBlock() in VPlan::execute() as done now, say because the creation of L is to be moved to later?

fhahn updated this revision to Diff 419087.Mar 30 2022, 3:01 AM
fhahn marked an inline comment as done.

Move initialization of State::CFG back to VPlan::execute.

fhahn added inline comments.Mar 30 2022, 3:04 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7590

Oh, no, sorry ... I meant LVP should (continue to) set the minimal amount of fields in State here,

Ah right that makes sense. I updated the patch to only set VectorPreHeader here and set the other fields in VPlan::execute based on that.

Ayal accepted this revision.Mar 30 2022, 4:41 AM

This is fine, thanks!

Couple of minor nits, plus Summary can be updated:
"use ILV::LoopMiddleBlock directly to set VPTransformState::CFG::LastBB" >>
"use successor of VectorHeaderBB (LoopMiddleBlock) directly to set VPTransformState::CFG::ExitBB"

llvm/lib/Transforms/Vectorize/VPlan.cpp
913–914

nit: (irrespective of this patch) can fold as L is used only here

State->CurrentVectorLoop = State->LI->getLoopFor(VectorHeaderBB);
914

nit: simpler and clearer to set

BasicBlock *VectorHeaderBB = State->CFG.VectorPreHeader->getSingleSuccessor();
State->CFG.PrevBB = VectorHeaderBB;
This revision is now accepted and ready to land.Mar 30 2022, 4:41 AM
fhahn updated this revision to Diff 419384.Mar 31 2022, 3:15 AM
fhahn marked 2 inline comments as done.

This is fine, thanks!

Couple of minor nits, plus Summary can be updated:
"use ILV::LoopMiddleBlock directly to set VPTransformState::CFG::LastBB" >>
"use successor of VectorHeaderBB (LoopMiddleBlock) directly to set VPTransformState::CFG::ExitBB"

Thanks, the nits should be addressedd in the latest version. I'll update the description and plan to land this soon.

llvm/lib/Transforms/Vectorize/VPlan.cpp
913–914

Done separately in 2c494f094123

914

Simplified as suggested, thanks!

fhahn edited the summary of this revision. (Show Details)Mar 31 2022, 3:16 AM
This revision was landed with ongoing or failed builds.Mar 31 2022, 3:49 AM
This revision was automatically updated to reflect the committed changes.