Skip to content
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

[VPlan] Manage created blocks directly in VPlan. (NFC) #120918

Merged
merged 12 commits into from
Dec 30, 2024
8 changes: 4 additions & 4 deletions llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2454,7 +2454,7 @@ static void introduceCheckBlockInVPlan(VPlan &Plan, BasicBlock *CheckIRBB) {
assert(PreVectorPH->getNumSuccessors() == 2 && "Expected 2 successors");
assert(PreVectorPH->getSuccessors()[0] == ScalarPH &&
"Unexpected successor");
VPIRBasicBlock *CheckVPIRBB = VPIRBasicBlock::fromBasicBlock(CheckIRBB);
VPIRBasicBlock *CheckVPIRBB = Plan.createVPIRBasicBlock(CheckIRBB);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Such construction of VPBB's seems complementary of VPBuilder, VPlanHCFGBuilder, VPRecipeBuilder?

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, but in this case it is directly tied to VPlan so we can track liveness conveniently; we could track liveness separately from VPlan, but then we would have 2 data structures to keep in sync and to delete together.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This thought can be pursued as follow-up.

A builder can be initialized with VPlan. The builder would provide an API for creating blocks (including but not limited to), whose implementation registers the new block within VPlan. Would be good to create VPlan components consistently, rather than having the abovementioned four distinct mechanisms.

VPBlockUtils::insertOnEdge(PreVectorPH, VectorPH, CheckVPIRBB);
PreVectorPH = CheckVPIRBB;
}
Expand Down Expand Up @@ -8084,11 +8084,11 @@ EpilogueVectorizerEpilogueLoop::emitMinimumVectorEpilogueIterCountCheck(

// A new entry block has been created for the epilogue VPlan. Hook it in, as
// otherwise we would try to modify the entry to the main vector loop.
VPIRBasicBlock *NewEntry = VPIRBasicBlock::fromBasicBlock(Insert);
VPIRBasicBlock *NewEntry = Plan.createVPIRBasicBlock(Insert);
VPBasicBlock *OldEntry = Plan.getEntry();
VPBlockUtils::reassociateBlocks(OldEntry, NewEntry);
Plan.setEntry(NewEntry);
delete OldEntry;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps worth leaving behind a note that OldEntry is now dead and will be collected later? Same with other deleted deletes.

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!

// OldEntry is now dead and will be cleaned up when the plan gets destroyed.

introduceCheckBlockInVPlan(Plan, Insert);
return Insert;
Expand Down Expand Up @@ -9289,7 +9289,7 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
VPBB->appendRecipe(Recipe);
}

VPBlockUtils::insertBlockAfter(new VPBasicBlock(), VPBB);
VPBlockUtils::insertBlockAfter(Plan->createVPBasicBlock(""), VPBB);
VPBB = cast<VPBasicBlock>(VPBB->getSingleSuccessor());
}

Expand Down
113 changes: 64 additions & 49 deletions llvm/lib/Transforms/Vectorize/VPlan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -205,11 +205,6 @@ VPBlockBase *VPBlockBase::getEnclosingBlockWithPredecessors() {
return Parent->getEnclosingBlockWithPredecessors();
}

void VPBlockBase::deleteCFG(VPBlockBase *Entry) {
for (VPBlockBase *Block : to_vector(vp_depth_first_shallow(Entry)))
delete Block;
}

VPBasicBlock::iterator VPBasicBlock::getFirstNonPhi() {
iterator It = begin();
while (It != end() && It->isPhi())
Expand Down Expand Up @@ -474,6 +469,13 @@ void VPIRBasicBlock::execute(VPTransformState *State) {
connectToPredecessors(State->CFG);
}

VPIRBasicBlock *VPIRBasicBlock::clone() {
auto *NewBlock = getPlan()->createEmptyVPIRBasicBlock(IRBB);
for (VPRecipeBase &R : Recipes)
NewBlock->appendRecipe(R.clone());
Comment on lines +474 to +475
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
for (VPRecipeBase &R : Recipes)
NewBlock->appendRecipe(R.clone());
for (VPRecipeBase &CurrentR : Recipes)
NewBlock->appendRecipe(CurrentR.clone());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New constructor should avoid need to rename.

return NewBlock;
}

void VPBasicBlock::execute(VPTransformState *State) {
bool Replica = bool(State->Lane);
BasicBlock *NewBB = State->CFG.PrevBB; // Reuse it if possible.
Expand Down Expand Up @@ -513,14 +515,11 @@ void VPBasicBlock::execute(VPTransformState *State) {
executeRecipes(State, NewBB);
}

void VPBasicBlock::dropAllReferences(VPValue *NewValue) {
for (VPRecipeBase &R : Recipes) {
for (auto *Def : R.definedValues())
Def->replaceAllUsesWith(NewValue);

for (unsigned I = 0, E = R.getNumOperands(); I != E; I++)
R.setOperand(I, NewValue);
}
VPBasicBlock *VPBasicBlock::clone() {
auto *NewBlock = getPlan()->createVPBasicBlock(getName());
for (VPRecipeBase &R : *this)
NewBlock->appendRecipe(R.clone());
return NewBlock;
}

void VPBasicBlock::executeRecipes(VPTransformState *State, BasicBlock *BB) {
Expand All @@ -541,7 +540,7 @@ VPBasicBlock *VPBasicBlock::splitAt(iterator SplitAt) {

SmallVector<VPBlockBase *, 2> Succs(successors());
// Create new empty block after the block to split.
auto *SplitBlock = new VPBasicBlock(getName() + ".split");
auto *SplitBlock = getPlan()->createVPBasicBlock(getName() + ".split");
VPBlockUtils::insertBlockAfter(SplitBlock, this);

// Finally, move the recipes starting at SplitAt to new block.
Expand Down Expand Up @@ -701,20 +700,13 @@ static std::pair<VPBlockBase *, VPBlockBase *> cloneFrom(VPBlockBase *Entry) {

VPRegionBlock *VPRegionBlock::clone() {
const auto &[NewEntry, NewExiting] = cloneFrom(getEntry());
auto *NewRegion =
new VPRegionBlock(NewEntry, NewExiting, getName(), isReplicator());
auto *NewRegion = getPlan()->createVPRegionBlock(NewEntry, NewExiting,
getName(), isReplicator());
for (VPBlockBase *Block : vp_depth_first_shallow(NewEntry))
Block->setParent(NewRegion);
return NewRegion;
}

void VPRegionBlock::dropAllReferences(VPValue *NewValue) {
for (VPBlockBase *Block : vp_depth_first_shallow(Entry))
// Drop all references in VPBasicBlocks and replace all uses with
// DummyValue.
Block->dropAllReferences(NewValue);
}

void VPRegionBlock::execute(VPTransformState *State) {
ReversePostOrderTraversal<VPBlockShallowTraversalWrapper<VPBlockBase *>>
RPOT(Entry);
Expand Down Expand Up @@ -822,32 +814,33 @@ void VPRegionBlock::print(raw_ostream &O, const Twine &Indent,
#endif

VPlan::VPlan(Loop *L) {
setEntry(VPIRBasicBlock::fromBasicBlock(L->getLoopPreheader()));
ScalarHeader = VPIRBasicBlock::fromBasicBlock(L->getHeader());
setEntry(createVPIRBasicBlock(L->getLoopPreheader()));
ScalarHeader = createVPIRBasicBlock(L->getHeader());
}

VPlan::~VPlan() {
if (Entry) {
VPValue DummyValue;
for (VPBlockBase *Block : vp_depth_first_shallow(Entry))
Block->dropAllReferences(&DummyValue);

VPBlockBase::deleteCFG(Entry);
}
for (auto *VPB : CreatedBlocks) {
if (auto *VPBB = dyn_cast<VPBasicBlock>(VPB)) {
// Replace all operands of recipes and all VPValues define in VPBB with
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
// Replace all operands of recipes and all VPValues define in VPBB with
// Replace all operands of recipes and all VPValues defined in VPBB with

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed thanks

// DummyValue so the block can be deleted.
for (VPRecipeBase &R : *VPBB) {
for (auto *Def : R.definedValues())
Def->replaceAllUsesWith(&DummyValue);

for (unsigned I = 0, E = R.getNumOperands(); I != E; I++)
R.setOperand(I, &DummyValue);
}
}
delete VPB;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can VPB be deleted immediately after dropping all references from its recipes (fusing the two loops together), or need to drop all references from recipes of all blocks first?

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, merged the loops. thanks

}
for (VPValue *VPV : VPLiveInsToFree)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: CreatedBlocks >> VPBlocksToFree would be more consistent with VPLiveInsToFree.

delete VPV;
if (BackedgeTakenCount)
delete BackedgeTakenCount;
}

VPIRBasicBlock *VPIRBasicBlock::fromBasicBlock(BasicBlock *IRBB) {
auto *VPIRBB = new VPIRBasicBlock(IRBB);
for (Instruction &I :
make_range(IRBB->begin(), IRBB->getTerminator()->getIterator()))
VPIRBB->appendRecipe(new VPIRInstruction(I));
return VPIRBB;
}

VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
PredicatedScalarEvolution &PSE,
bool RequiresScalarEpilogueCheck,
Expand All @@ -861,7 +854,7 @@ VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
// an epilogue vector loop, the original entry block here will be replaced by
// a new VPIRBasicBlock wrapping the entry to the epilogue vector loop after
// generating code for the main vector loop.
VPBasicBlock *VecPreheader = new VPBasicBlock("vector.ph");
VPBasicBlock *VecPreheader = Plan->createVPBasicBlock("vector.ph");
VPBlockUtils::connectBlocks(Plan->getEntry(), VecPreheader);

// Create SCEV and VPValue for the trip count.
Expand All @@ -878,17 +871,17 @@ VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,

// Create VPRegionBlock, with empty header and latch blocks, to be filled
// during processing later.
VPBasicBlock *HeaderVPBB = new VPBasicBlock("vector.body");
VPBasicBlock *LatchVPBB = new VPBasicBlock("vector.latch");
VPBasicBlock *HeaderVPBB = Plan->createVPBasicBlock("vector.body");
VPBasicBlock *LatchVPBB = Plan->createVPBasicBlock("vector.latch");
VPBlockUtils::insertBlockAfter(LatchVPBB, HeaderVPBB);
auto *TopRegion = new VPRegionBlock(HeaderVPBB, LatchVPBB, "vector loop",
false /*isReplicator*/);
auto *TopRegion = Plan->createVPRegionBlock(
HeaderVPBB, LatchVPBB, "vector loop", false /*isReplicator*/);

VPBlockUtils::insertBlockAfter(TopRegion, VecPreheader);
VPBasicBlock *MiddleVPBB = new VPBasicBlock("middle.block");
VPBasicBlock *MiddleVPBB = Plan->createVPBasicBlock("middle.block");
VPBlockUtils::insertBlockAfter(MiddleVPBB, TopRegion);

VPBasicBlock *ScalarPH = new VPBasicBlock("scalar.ph");
VPBasicBlock *ScalarPH = Plan->createVPBasicBlock("scalar.ph");
VPBlockUtils::connectBlocks(ScalarPH, ScalarHeader);
if (!RequiresScalarEpilogueCheck) {
VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH);
Expand All @@ -904,7 +897,7 @@ VPlanPtr VPlan::createInitialVPlan(Type *InductionTy,
// we unconditionally branch to the scalar preheader. Do nothing.
// 3) Otherwise, construct a runtime check.
BasicBlock *IRExitBlock = TheLoop->getUniqueLatchExitBlock();
auto *VPExitBlock = VPIRBasicBlock::fromBasicBlock(IRExitBlock);
auto *VPExitBlock = Plan->createVPIRBasicBlock(IRExitBlock);
// The connection order corresponds to the operands of the conditional branch.
VPBlockUtils::insertBlockAfter(VPExitBlock, MiddleVPBB);
VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH);
Expand Down Expand Up @@ -960,15 +953,13 @@ void VPlan::prepareToExecute(Value *TripCountV, Value *VectorTripCountV,
/// have a single predecessor, which is rewired to the new VPIRBasicBlock. All
/// successors of VPBB, if any, are rewired to the new VPIRBasicBlock.
static void replaceVPBBWithIRVPBB(VPBasicBlock *VPBB, BasicBlock *IRBB) {
VPIRBasicBlock *IRVPBB = VPIRBasicBlock::fromBasicBlock(IRBB);
VPIRBasicBlock *IRVPBB = VPBB->getPlan()->createVPIRBasicBlock(IRBB);
for (auto &R : make_early_inc_range(*VPBB)) {
assert(!R.isPhi() && "Tried to move phi recipe to end of block");
R.moveBefore(*IRVPBB, IRVPBB->end());
}

VPBlockUtils::reassociateBlocks(VPBB, IRVPBB);
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
VPBlockUtils::reassociateBlocks(VPBB, IRVPBB);
VPBlockUtils::reassociateBlocks(VPBB, IRVPBB);
// VPBB is now dead and will be cleaned up when the plan gets destroyed.

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


delete VPBB;
}

/// Generate the code inside the preheader and body of the vectorized loop.
Expand Down Expand Up @@ -1217,6 +1208,7 @@ static void remapOperands(VPBlockBase *Entry, VPBlockBase *NewEntry,
}

VPlan *VPlan::duplicate() {
unsigned NumBlocksBeforeCloning = CreatedBlocks.size();
// Clone blocks.
const auto &[NewEntry, __] = cloneFrom(Entry);

Expand Down Expand Up @@ -1257,9 +1249,32 @@ VPlan *VPlan::duplicate() {
assert(Old2NewVPValues.contains(TripCount) &&
"TripCount must have been added to Old2NewVPValues");
NewPlan->TripCount = Old2NewVPValues[TripCount];

// Transfer all cloned blocks (the second half of all current blocks) from
// current to new VPlan.
unsigned NumBlocksAfterCloning = CreatedBlocks.size();
for (unsigned I :
seq<unsigned>(NumBlocksBeforeCloning, NumBlocksAfterCloning))
NewPlan->CreatedBlocks.push_back(this->CreatedBlocks[I]);
CreatedBlocks.truncate(NumBlocksBeforeCloning);

return NewPlan;
}

VPIRBasicBlock *VPlan::createEmptyVPIRBasicBlock(BasicBlock *IRBB) {
auto *VPIRBB = new VPIRBasicBlock(IRBB);
CreatedBlocks.push_back(VPIRBB);
return VPIRBB;
}

VPIRBasicBlock *VPlan::createVPIRBasicBlock(BasicBlock *IRBB) {
auto *VPIRBB = createEmptyVPIRBasicBlock(IRBB);
for (Instruction &I :
make_range(IRBB->begin(), IRBB->getTerminator()->getIterator()))
VPIRBB->appendRecipe(new VPIRInstruction(I));
return VPIRBB;
}

#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)

Twine VPlanPrinter::getUID(const VPBlockBase *Block) {
Expand Down
Loading
Loading