Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions llvm/lib/Transforms/Vectorize/VPlan.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ class VPBlockBase {
Predecessors.erase(Pos);
}

public:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hopefully this low-level API can remain private, possibly with an addition to the high-level public API, see below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done thanks!

/// Remove \p Successor from the successors of this block.
void removeSuccessor(VPBlockBase *Successor) {
auto Pos = find(Successors, Successor);
Expand All @@ -129,8 +130,6 @@ class VPBlockBase {
void replacePredecessor(VPBlockBase *Old, VPBlockBase *New) {
auto I = find(Predecessors, Old);
assert(I != Predecessors.end());
assert(Old->getParent() == New->getParent() &&
"replaced predecessor must have the same parent");
*I = New;
}

Expand Down
78 changes: 38 additions & 40 deletions llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ VPBasicBlock *PlainCFGBuilder::getOrCreateVPBB(BasicBlock *BB) {
return VPBB;
}

if (!TheLoop->contains(BB))
return Plan->getExitBlock(BB);

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about populating BB2VPBB with Plan's exit blocks on PlainCFGBuilder's construction, to fold this check with the one above?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done thanks!

// Create new VPBB.
StringRef Name = BB->getName();
LLVM_DEBUG(dbgs() << "Creating VPBasicBlock for " << Name << "\n");
Expand Down Expand Up @@ -146,14 +149,6 @@ bool PlainCFGBuilder::isExternalDef(Value *Val) {
// Instruction definition is in outermost loop PH.
return false;

// Check whether Instruction definition is in a loop exit.
SmallVector<BasicBlock *> ExitBlocks;
TheLoop->getExitBlocks(ExitBlocks);
if (is_contained(ExitBlocks, InstParent)) {
// Instruction definition is in outermost loop exit.
return false;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this simply redundant, independently, as such instructions which reside in ExitBlocks are not contained in TheLoop?

Should the above definition of external defs be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, removed as NFC in 282af2d

// Check whether Instruction definition is in loop body.
return !TheLoop->contains(Inst);
}
Expand Down Expand Up @@ -202,11 +197,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()) {
Copy link
Collaborator

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?

Suggested change
if (Br->isConditional()) {
if (!Br->isConditional())
continue;

Copy link
Collaborator

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.

Expand Down Expand Up @@ -296,7 +286,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);

Expand Down Expand Up @@ -327,24 +316,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.
Expand Down Expand Up @@ -447,19 +424,21 @@ static void createLoopRegion(VPlan &Plan, VPBlockBase *HeaderVPB) {
VPBlockBase *Succ = LatchVPBB->getSingleSuccessor();
assert(LatchVPBB->getNumSuccessors() <= 1 &&
"Latch has more than one successor");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert(LatchVPBB->getNumSuccessors() <= 1 &&
"Latch has more than one successor");
assert(Succ && "Latch expected to be left with a single successor");

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done thanks!

if (Succ)
VPBlockUtils::disconnectBlocks(LatchVPBB, Succ);
LatchVPBB->removeSuccessor(Succ);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The constructor of VPRegionBlock will assert that LatchVPBB is free of successors (and HeaderVPB free of predecessors), yet we want to retain LatchVPBB's place in Succ's predecessors - to be replaced by R, the region itself. One possible way to achieve this using current public API may by using an auxiliary temporary place holder VPBB:

Suggested change
LatchVPBB->removeSuccessor(Succ);
auto *PlaceHolder = Plan.createVPBasicBlock ("Region place holder");
VPBlockUtils::insertOnEdge(LatchVPBB, Succ, PlaceHolder);
VPBlockUtils::disconnectBlocks(LatchVPBB, PlaceHolder);
VPBlockUtils::connectBlocks(PreheaderVPBB, PlaceHolder);

Note that disconnecting LatchVPBB from PlaceHolder and connecting PreheaderVPBB to PlaceHolder instead retains the position in PlaceHolder's predecessors, as PlaceHolder has but one predecessor (unlike Succ). Plus PreheaderVPBB has but one successor. More on this later.

It is also conceivable to have a(nother) constructor of VPRegionBlock which accepts a Header and Latch that are connected to a Preheader and Exit/Succ, respectively, and takes care of hooking them up with the newly created region block instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, introducing a temporary block nicely solves the issue, thank you very much!

This also required changing the region construction order to innermost first, to ensure the parents for VPBBs are set correctly at all times 32928a0


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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above explanation needs update - can now also reach exit blocks from HeaderVPB - via early exits?
Latch isn't fully disconnected yet its successors are, which should still prevent reaching an exit block from it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated the comment to refer to non-latch exiting blocks.

SmallPtrSet<VPBlockBase *, 2> ExitBlocks(Plan.getExitBlocks().begin(),
Plan.getExitBlocks().end());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SmallPtrSet<VPBlockBase *, 2> ExitBlocks(Plan.getExitBlocks().begin(),
Plan.getExitBlocks().end());

?

for (VPBlockBase *VPBB : vp_depth_first_shallow(HeaderVPB))
VPBB->setParent(R);
if (!ExitBlocks.contains(VPBB))
Copy link
Collaborator

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, comment updated, thanks!

Copy link
Collaborator

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?

Copy link
Contributor Author

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!

VPBB->setParent(R);

VPBlockUtils::insertBlockAfter(R, PreheaderVPBB);
Copy link
Collaborator

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

Suggested change
VPBlockUtils::insertBlockAfter(R, PreheaderVPBB);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done thanks

if (Succ)
VPBlockUtils::connectBlocks(R, Succ);
R->setOneSuccessor(Succ);
Succ->replacePredecessor(LatchVPBB, R);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
R->setOneSuccessor(Succ);
Succ->replacePredecessor(LatchVPBB, R);
VPBlockUtils::insertOnEdge(PlaceHolder, Succ, R);

followed by either letting mergeBlocksIntoPredecessors() eliminate the empty PlaceHolder by merging it into PreheaderVPBB, or doing so explicitly here by adding:

  VPBlockUtils::disconnectBlocks(PreheaderVPBB, PlaceHolder);
  VPBlockUtils::disconnectBlocks(PlaceHolder, R);
  VPBlockUtils::connectBlocks(PreheaderVPBB, R);

Note that doing

  VPBlockUtils::insertOnEdge(PreheaderVPBB, RegionPlaceHolder, R);

instead will lose LatchVPBB's position in Succ's predecessors, suggesting that mergeBlocksIntoPredecessors() needs to be improved to retain this position (TODO). Should we have a removeOnEdge() which takes care of removing a Block on an edge from From to To, making sure From will appear as the predecessor of To in place of Block, and To will appear as the successor of From in place of Block? Analogous to insertOnEdge(), which utilizes connectBlocks(From, To, PredIx, SuccIdx), to ensure such positions are retained.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I went with removing the temporary block

}

// Add the necessary canonical IV and branch recipes required to control the
Expand Down Expand Up @@ -511,12 +490,33 @@ void VPlanTransforms::prepareForVectorization(VPlan &Plan, Type *InductionTy,
VPBlockUtils::insertBlockAfter(VecPreheader, Plan.getEntry());

VPBasicBlock *MiddleVPBB = Plan.createVPBasicBlock("middle.block");
VPBlockUtils::connectBlocks(LatchVPB, MiddleVPBB);
LatchVPB->swapSuccessors();
VPBlockBase *LatchExitVPB = LatchVPB->getNumSuccessors() == 2
? LatchVPB->getSuccessors()[0]
: nullptr;
if (LatchExitVPB) {
LatchVPB->getSuccessors()[0] = MiddleVPBB;
MiddleVPBB->setPredecessors({LatchVPB});
MiddleVPBB->setSuccessors({LatchExitVPB});
LatchExitVPB->replacePredecessor(LatchVPB, MiddleVPBB);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
VPBlockBase *LatchExitVPB = LatchVPB->getNumSuccessors() == 2
? LatchVPB->getSuccessors()[0]
: nullptr;
if (LatchExitVPB) {
LatchVPB->getSuccessors()[0] = MiddleVPBB;
MiddleVPBB->setPredecessors({LatchVPB});
MiddleVPBB->setSuccessors({LatchExitVPB});
LatchExitVPB->replacePredecessor(LatchVPB, MiddleVPBB);
// Canonical LatchVPB has header block as last successor. If it has another
// successor, the latter is an exit block - insert middle block on its edge.
// Otherwise, add middle block as another successor retaining header as last.
if (LatchVPB->getNumSuccessors() == 2) {
VPBlockBase *LatchExitVPB = LatchVPB->getSuccessors()[0];
VPBlockUtils::insertOnEdge(LatchVPB, LatchExitVPB, MiddleVPBB);

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done thanks!

} else {
VPBlockUtils::connectBlocks(LatchVPB, MiddleVPBB);
LatchVPB->swapSuccessors();
}

addCanonicalIVRecipes(Plan, cast<VPBasicBlock>(HeaderVPB),
cast<VPBasicBlock>(LatchVPB), InductionTy, IVDL);

// Disconnect all edges between exit blocks other than from the latch.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Disconnect all edges between exit blocks other than from the latch.
// Disconnect all edges to exit blocks other than from the middle block.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done thanks!

// TODO: Uncountable exit blocks should be handled here.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are "handled", being listed in getExitBlocks; but should be handled differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, here for now we just simply drop the edges, i.e. leaving the plan in an incorrect/incomplete state, relying on the later transform to adjust the exit condition and introduce an extra middle block, looking up the early exit condition using original IR references.

for (VPBlockBase *EB : to_vector(Plan.getExitBlocks())) {
for (VPBlockBase *Pred : to_vector(EB->getPredecessors())) {
if (Pred == MiddleVPBB)
continue;
cast<VPBasicBlock>(Pred)->getTerminator()->eraseFromParent();
VPBlockUtils::disconnectBlocks(Pred, EB);
Copy link
Contributor

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

}
}

// Create SCEV and VPValue for the trip count.
// We use the symbolic max backedge-taken-count, which works also when
// vectorizing loops with uncountable early exits.
Expand All @@ -541,8 +541,9 @@ void VPlanTransforms::prepareForVectorization(VPlan &Plan, Type *InductionTy,
// Thus if tail is to be folded, we know we don't need to run the
// remainder and we can set the condition to true.
// 3) Otherwise, construct a runtime check.

if (!RequiresScalarEpilogueCheck) {
if (LatchExitVPB)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (LatchExitVPB)
if (auto *LatchExitVPB = MiddleVPBB->getSingleSuccessor())

?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done thanks!

VPBlockUtils::disconnectBlocks(MiddleVPBB, LatchExitVPB);
VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH);
// The exit blocks are unreachable, remove their recipes to make sure no
// users remain that may pessimize transforms.
Expand All @@ -554,9 +555,6 @@ void VPlanTransforms::prepareForVectorization(VPlan &Plan, Type *InductionTy,
}

// The connection order corresponds to the operands of the conditional branch.
Copy link
Collaborator

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."?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added thanks

BasicBlock *IRExitBlock = TheLoop->getUniqueLatchExitBlock();
auto *VPExitBlock = Plan.getExitBlock(IRExitBlock);
VPBlockUtils::connectBlocks(MiddleVPBB, VPExitBlock);
VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH);

auto *ScalarLatchTerm = TheLoop->getLoopLatch()->getTerminator();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,11 @@ define void @foo(i64 %n) {
; CHECK-NEXT: outer.latch:
; CHECK-NEXT: EMIT ir<%outer.iv.next> = add ir<%outer.iv>, ir<1>
; CHECK-NEXT: EMIT ir<%outer.ec> = icmp ir<%outer.iv.next>, ir<8>
; CHECK-NEXT: Successor(s): outer.header
; CHECK-NEXT: EMIT branch-on-cond ir<%outer.ec>
; CHECK-NEXT: Successor(s): ir-bb<exit>, outer.header
; CHECK-EMPTY:
; CHECK-NEXT: ir-bb<exit>:
; CHECK-NEXT: No successors
; CHECK-NEXT: }
entry:
br label %outer.header
Expand Down
Loading