This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Verify plan entry and exit blocks, set correct exit block.
ClosedPublic

Authored by fhahn on Nov 25 2021, 5:38 AM.

Details

Summary

Both the entry and exit blocks of the top-region of a plan must be
VPBasicBlocks. They also must have no predecessors or successors
respectively.

This invariant was broken when splitting a block for sink-after. To fix
the issue, set the exit block of the region *after* sink-after is done.

Diff Detail

Event Timeline

fhahn created this revision.Nov 25 2021, 5:38 AM
fhahn requested review of this revision.Nov 25 2021, 5:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 25 2021, 5:38 AM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added inline comments.Dec 2 2021, 1:42 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9167–9168

Wonder if the above assert should also be taken care of by VPlan verify instead?

9238

Another way of fixing this would be to have split-blocks update Exit, when it updates VPBB?

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
169

The Entry and Exit blocks of any region in Plan must have no predecessors and successors, respectively, by definition; not only the top-most region, e.g., including internal replicating regions. Worth extending to check all regions in Plan? Entry and Exit blocks may themselves be regions, in general, so asserting that they be basic blocks should be restricted to the top-most region.

fhahn updated this revision to Diff 391921.Dec 5 2021, 9:23 AM

Verify empty predecessors/successors for all region entries/exits.

fhahn marked 3 inline comments as done.Dec 5 2021, 9:26 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9167–9168

Could do, the only missing piece would be the non-empty part. I think that makes sense here, but might be overly restrictive as general requirement?

9238

Yes, I can change it, but I think that be a bit more work.

llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp
169

Done, I updated the code to verify empty predecessors/successor for all region entries/exits.

Ayal accepted this revision.Dec 5 2021, 11:32 AM

This is fine! Adding a follow-up thought.

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

(Continuing to reason about the above other way, independent of this patch which is fine - ) would it make sense to maintain a valid Region throughout, i.e., create it with an Entry and Exit upfront? That is, with a non-empty Entry following D111299, connected to an initially empty Exit, and where intra-loop code/blocks are to be introduced between VPBB and Exit (except for code seeking to be introduced in the latch/Exit..). Admittedly refers to D113182. Or would doing so interfere with how D111299 et al. seek to model/build the vector latch, preheader and exit blocks in VPlan.

This revision is now accepted and ready to land.Dec 5 2021, 11:32 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked 3 inline comments as done.
fhahn marked an inline comment as done.Dec 7 2021, 8:30 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9238

I don't think that would cause any major issues I think, just a potentially redundant block in the plan, if create a dedicated exit block up front.