-
Notifications
You must be signed in to change notification settings - Fork 15.9k
[VPlan] Retain exit conditions and edges in initial VPlan (NFC). #137709
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
de0fc2d
19bec82
55b4d5e
ca45f88
ddfdeef
1284bc6
79317a5
3cc4b31
bbb902e
c1902ed
0cadcf9
5fa6b7d
5d6729d
1211faf
50d7ee1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -182,11 +182,6 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB, | |||||||||||||
| "Instruction shouldn't have been visited."); | ||||||||||||||
|
|
||||||||||||||
| if (auto *Br = dyn_cast<BranchInst>(Inst)) { | ||||||||||||||
| if (TheLoop->getLoopLatch() == BB || | ||||||||||||||
| any_of(successors(BB), | ||||||||||||||
| [this](BasicBlock *Succ) { return !TheLoop->contains(Succ); })) | ||||||||||||||
| continue; | ||||||||||||||
|
|
||||||||||||||
| // Conditional branch instruction are represented using BranchOnCond | ||||||||||||||
| // recipes. | ||||||||||||||
| if (Br->isConditional()) { | ||||||||||||||
|
|
@@ -251,6 +246,8 @@ std::unique_ptr<VPlan> PlainCFGBuilder::buildPlainCFG( | |||||||||||||
| DenseMap<VPBlockBase *, BasicBlock *> &VPB2IRBB) { | ||||||||||||||
| VPIRBasicBlock *Entry = cast<VPIRBasicBlock>(Plan->getEntry()); | ||||||||||||||
| BB2VPBB[Entry->getIRBasicBlock()] = Entry; | ||||||||||||||
| for (VPIRBasicBlock *ExitVPBB : Plan->getExitBlocks()) | ||||||||||||||
| BB2VPBB[ExitVPBB->getIRBasicBlock()] = ExitVPBB; | ||||||||||||||
|
|
||||||||||||||
| // 1. Scan the body of the loop in a topological order to visit each basic | ||||||||||||||
| // block after having visited its predecessor basic blocks. Create a VPBB for | ||||||||||||||
|
|
@@ -276,7 +273,6 @@ std::unique_ptr<VPlan> PlainCFGBuilder::buildPlainCFG( | |||||||||||||
| for (BasicBlock *BB : RPO) { | ||||||||||||||
| // Create or retrieve the VPBasicBlock for this BB. | ||||||||||||||
| VPBasicBlock *VPBB = getOrCreateVPBB(BB); | ||||||||||||||
| Loop *LoopForBB = LI->getLoopFor(BB); | ||||||||||||||
| // Set VPBB predecessors in the same order as they are in the incoming BB. | ||||||||||||||
| setVPBBPredsFromBB(VPBB, BB); | ||||||||||||||
|
|
||||||||||||||
|
|
@@ -307,24 +303,12 @@ std::unique_ptr<VPlan> PlainCFGBuilder::buildPlainCFG( | |||||||||||||
| BasicBlock *IRSucc1 = BI->getSuccessor(1); | ||||||||||||||
| VPBasicBlock *Successor0 = getOrCreateVPBB(IRSucc0); | ||||||||||||||
| VPBasicBlock *Successor1 = getOrCreateVPBB(IRSucc1); | ||||||||||||||
|
|
||||||||||||||
| // Don't connect any blocks outside the current loop except the latches for | ||||||||||||||
| // inner loops. | ||||||||||||||
| // TODO: Also connect exit blocks during initial VPlan construction. | ||||||||||||||
| if (LoopForBB == TheLoop || BB != LoopForBB->getLoopLatch()) { | ||||||||||||||
| if (!LoopForBB->contains(IRSucc0)) { | ||||||||||||||
| VPBB->setOneSuccessor(Successor1); | ||||||||||||||
| continue; | ||||||||||||||
| } | ||||||||||||||
| if (!LoopForBB->contains(IRSucc1)) { | ||||||||||||||
| VPBB->setOneSuccessor(Successor0); | ||||||||||||||
| continue; | ||||||||||||||
| } | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| VPBB->setTwoSuccessors(Successor0, Successor1); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| for (auto *EB : Plan->getExitBlocks()) | ||||||||||||||
| setVPBBPredsFromBB(EB, EB->getIRBasicBlock()); | ||||||||||||||
|
|
||||||||||||||
| // 2. The whole CFG has been built at this point so all the input Values must | ||||||||||||||
| // have a VPlan counterpart. Fix VPlan header phi by adding their | ||||||||||||||
| // corresponding VPlan operands. | ||||||||||||||
|
|
@@ -425,21 +409,31 @@ static void createLoopRegion(VPlan &Plan, VPBlockBase *HeaderVPB) { | |||||||||||||
| VPBlockUtils::disconnectBlocks(PreheaderVPBB, HeaderVPB); | ||||||||||||||
| VPBlockUtils::disconnectBlocks(LatchVPBB, HeaderVPB); | ||||||||||||||
| VPBlockBase *Succ = LatchVPBB->getSingleSuccessor(); | ||||||||||||||
| assert(LatchVPBB->getNumSuccessors() <= 1 && | ||||||||||||||
| "Latch has more than one successor"); | ||||||||||||||
| if (Succ) | ||||||||||||||
| VPBlockUtils::disconnectBlocks(LatchVPBB, Succ); | ||||||||||||||
| assert(Succ && "Latch expected to be left with a single successor"); | ||||||||||||||
|
|
||||||||||||||
| // Use a temporary placeholder between LatchVPBB and its successor, to | ||||||||||||||
| // preserve the original predecessor/successor order of the blocks. | ||||||||||||||
| auto *PlaceHolder = Plan.createVPBasicBlock("Region place holder"); | ||||||||||||||
| VPBlockUtils::insertOnEdge(LatchVPBB, Succ, PlaceHolder); | ||||||||||||||
| VPBlockUtils::disconnectBlocks(LatchVPBB, PlaceHolder); | ||||||||||||||
| VPBlockUtils::connectBlocks(PreheaderVPBB, PlaceHolder); | ||||||||||||||
|
|
||||||||||||||
| auto *R = Plan.createVPRegionBlock(HeaderVPB, LatchVPBB, "", | ||||||||||||||
| false /*isReplicator*/); | ||||||||||||||
| // All VPBB's reachable shallowly from HeaderVPB belong to top level loop, | ||||||||||||||
| // because VPlan is expected to end at top level latch disconnected above. | ||||||||||||||
| // All VPBB's reachable shallowly from HeaderVPB belong to the current region, | ||||||||||||||
| // except the exit blocks reachable via non-latch exiting blocks, | ||||||||||||||
|
||||||||||||||
| // except the exit blocks reachable via non-latch exiting blocks, | |
| // except the exit blocks reachable via non-latch exiting blocks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed thanks!
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| SmallPtrSet<VPBlockBase *, 2> ExitBlocks(Plan.getExitBlocks().begin(), | |
| Plan.getExitBlocks().end()); |
?
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can now reach exit blocks, contrary to above comment, via early exits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, comment updated, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm2, createLoopRegion() is called after prepareForVectorization(), which according to the changes below should have removed all early-exit edges, so is this check (if VPBB is an exit block) needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not reachable in the latest version, removed thanks!
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PlaceHolder introduced earlier may now be used instead as follows
| VPBlockUtils::insertBlockAfter(R, PreheaderVPBB); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done thanks
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inserting R first after Preheader (clearer to instead explicitly insert R on the edge from Preheader to PlaceHolder?) and then inserting R again on the edge between PlaceHolder and Succ seems a bit confusing, thereby creating bidirectional edges between R and PlaceHolder which are then removed. It's like inserting R both before PlaceHolder and after it, but both these insertions require R to be disconnected. In essence we want to replace PlaceHolder with R.
Another alternative is to have an empty region as a placeholder rather than an empty basic block, and then move/copy R's entry and exit blocks into it:
auto *PlaceHolder = Plan.createVPRegionBlock("", false /*isReplicator*/);
followed by
PlaceHolder.setEntry(R.getEntry());
PlaceHolder.setExit(R.getExit());
essentially turning R to be the temporary block and PlaceHolder to be R, constructed outside in rather than inside out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps suffice to be a bit clearer by slightly reordering (would Exit be a better name for Succ?):
// Have R replace PlaceHolder as successor of Preheader.
BlockUtils::insertOnEdge(PreheaderBlock, PlaceHolder, R);
BlockUtils::disconnectBlocks(R, PlaceHolder);
// Have R replace PlaceHolder as predecessor of Exit.
BlockUtils::insertOnEdge(PlaceHolder, Exit, R);
BlockUtils::disconnectBlocks(PlaceHolder, R);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually there's no need to construct a placeholder region, we can simply create an empty main region first and then set entry/exiting after adjusting the CFG. Updated, thanks!
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm a bit confused by the word "latter" here. Does "latter" refer to "another successor"? Perhaps it's easier just to say that if the latchvpb is not canonical the early exit block(s) come first, with the (canonical?) exit to the middle block being last? If I've understood the layout correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LatchVPB is canonical - as a result of calling canonicalHeaderAndLatch() above. Being a canonical latch means LatchVPB has header block as its last successor, and this property is maintained. If LatchVPB has another successor, in addition to header, this other successor (appears first and) is an exit block. In this case middle block is inserted on the edge from LatchVPB to its first exit block successor. Should "the latter" be replaced with "this other successor"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to say
If it has another successor, this successor is an exit block
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // TODO: VPlans with early exits should be explicitly converted to a form only | |
| // exiting via the latch here, including adjusting the exit condition, instead | |
| // of simplify disconnecting the edges and adjusting the VPlan later. | |
| // TODO: VPlans with early exits should be explicitly converted to a form | |
| // exiting only via the latch here, including adjusting the exit condition, | |
| // instead of simply disconnecting the edges and adjusting the VPlan later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a bit odd at first as it seems to be undoing what you did above with VPBlockUtils::insertOnEdge(LatchVPB, LatchExitVPB, MiddleVPBB);. I presume that's because the vector.early.exit VPBB sits between the latch block and the original IR early exit block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The edge from MiddleVPBB to LatchExitVPB is retained here explicitly via the early-continue exclusion. The edges removed here are early-exits from non-latch Pred block to early.exit block. Block vector.early.exit is introduced by handleUncountableEarlyExit() which currently takes place later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above with insertOnEdge we only handle the edge exiting from the latch. The exit block via the latch will now be connected to the middle block.
The loop here disconnects all early exits and they will be handled later: either by requiring at least one scalar iteration, nothing more needs to be done, or introducing the early exit control flow to go to the early exit via the additional middle block. For the latter case, the VPlan is now incomplete/incorrect.
To avoid this, we should directly handle the uncountable early exits here, which is done in #138393. This way, we do not need to rely on IR references in handleUncountableEarlyExit and the VPlan remains complete/correct throughout.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"// with middle block already connected to exit block."?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: seems a bit clearer to early continue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, ignore, we continue also after handling a conditional branch.