Skip to content

[VPlan] Make canonical IV part of the region #156262

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[VPlan] Make canonical IV part of the region #156262

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6391ca9
[VPlan] Don't reset canonical IV start value.
fhahn Aug 29, 2025
9361797
[VPlan] Make canonical IV part of the region
fhahn Jun 5, 2025
c709ab9
!fixup address latest comments, thanks
fhahn Oct 5, 2025
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
34 changes: 17 additions & 17 deletions llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4031,7 +4031,6 @@ static bool willGenerateVectors(VPlan &Plan, ElementCount VF,
case VPDef::VPScalarIVStepsSC:
case VPDef::VPReplicateSC:
case VPDef::VPInstructionSC:
case VPDef::VPCanonicalIVPHISC:
case VPDef::VPVectorPointerSC:
case VPDef::VPVectorEndPointerSC:
case VPDef::VPExpandSCEVSC:
Expand Down Expand Up @@ -8455,6 +8454,7 @@ VPlanPtr LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(
m_Specific(Plan->getCanonicalIV()), m_VPValue())) &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Above is another instance which could use Plan->getCanonicalIVInv(), possibly as a first step towards turning it into a region value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, split off to #163020

"Did not find the canonical IV increment");
cast<VPRecipeWithIRFlags>(IVInc)->dropPoisonGeneratingFlags();
Plan->getCanonicalIVInfo().HasNUW = false;
}

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -8518,8 +8518,7 @@ VPlanPtr LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(
// latter are added above for masking.
// FIXME: Migrate code relying on the underlying instruction from VPlan0
// to construct recipes below to not use the underlying instruction.
if (isa<VPCanonicalIVPHIRecipe, VPWidenCanonicalIVRecipe, VPBlendRecipe>(
&R) ||
if (isa<VPWidenCanonicalIVRecipe, VPBlendRecipe>(&R) ||
(isa<VPInstruction>(&R) && !UnderlyingValue))
continue;

Expand Down Expand Up @@ -8707,8 +8706,6 @@ VPlanPtr LoopVectorizationPlanner::tryToBuildVPlan(VFRange &Range) {
VPRecipeBuilder RecipeBuilder(*Plan, OrigLoop, TLI, &TTI, Legal, CM, PSE,
Builder, BlockMaskCache, nullptr /*LVer*/);
for (auto &R : Plan->getVectorLoopRegion()->getEntryBasicBlock()->phis()) {
if (isa<VPCanonicalIVPHIRecipe>(&R))
continue;
auto *HeaderR = cast<VPHeaderPHIRecipe>(&R);
RecipeBuilder.setRecipe(HeaderR->getUnderlyingInstr(), HeaderR);
}
Expand Down Expand Up @@ -9458,8 +9455,6 @@ static void preparePlanForMainVectorLoop(VPlan &MainPlan, VPlan &EpiPlan) {
SmallPtrSet<PHINode *, 2> EpiWidenedPhis;
for (VPRecipeBase &R :
EpiPlan.getVectorLoopRegion()->getEntryBasicBlock()->phis()) {
if (isa<VPCanonicalIVPHIRecipe>(&R))
continue;
EpiWidenedPhis.insert(
cast<PHINode>(R.getVPSingleValue()->getUnderlyingValue()));
}
Expand Down Expand Up @@ -9520,8 +9515,9 @@ static void preparePlanForMainVectorLoop(VPlan &MainPlan, VPlan &EpiPlan) {
VPPhi *ResumePhi = nullptr;
if (ResumePhiIter == MainScalarPH->phis().end()) {
VPBuilder ScalarPHBuilder(MainScalarPH, MainScalarPH->begin());
Type *Ty = VPTypeAnalysis(MainPlan).inferScalarType(VectorTC);
ResumePhi = ScalarPHBuilder.createScalarPhi(
{VectorTC, MainPlan.getCanonicalIV()->getStartValue()}, {},
{VectorTC, MainPlan.getOrAddLiveIn(Constant::getNullValue(Ty))}, {},
"vec.epilog.resume.val");
} else {
ResumePhi = cast<VPPhi>(&*ResumePhiIter);
Expand Down Expand Up @@ -9549,13 +9545,11 @@ static SmallVector<Instruction *> preparePlanForEpilogueVectorLoop(
VPBasicBlock *Header = VectorLoop->getEntryBasicBlock();
Header->setName("vec.epilog.vector.body");

// Ensure that the start values for all header phi recipes are updated before
// vectorizing the epilogue loop.
VPCanonicalIVPHIRecipe *IV = Plan.getCanonicalIV();
// When vectorizing the epilogue loop, the canonical induction start
// value needs to be changed from zero to the value after the main
// vector loop. Find the resume value created during execution of the main
// VPlan. It must be the first phi in the loop preheader.
// When vectorizing the epilogue loop, the canonical induction needs to be
// adjusted by the value after the main vector loop. Find the resume value
// created during execution of the main VPlan. It must be the first phi in the
// loop preheader. Use the value to increment the canonical IV, and update all
// users in the loop region to use the adjusted value.
// FIXME: Improve modeling for canonical IV start values in the epilogue
// loop.
Comment on lines 9553 to 9554
Copy link
Collaborator

Choose a reason for hiding this comment

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

Time to remove this FIXME or is further improvement desired?

using namespace llvm::PatternMatch;
Expand All @@ -9580,6 +9574,7 @@ static SmallVector<Instruction *> preparePlanForEpilogueVectorLoop(
EPI.VectorTripCount = EPResumeVal->getOperand(0);
}
VPValue *VPV = Plan.getOrAddLiveIn(EPResumeVal);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Independent: better rename VPV

VPValue *IV = VectorLoop->getCanonicalIV();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This replaces an if (auto *IV = dyn_cast...

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 originally part of the loop

assert(all_of(IV->users(),
[](const VPUser *U) {
return isa<VPScalarIVStepsRecipe>(U) ||
Expand All @@ -9590,11 +9585,16 @@ static SmallVector<Instruction *> preparePlanForEpilogueVectorLoop(
}) &&
"the canonical IV should only be used by its increment or "
"ScalarIVSteps when resetting the start value");
IV->setOperand(0, VPV);
VPBuilder Builder(Header, Header->getFirstNonPhi());
VPInstruction *Add = Builder.createNaryOp(Instruction::Add, {IV, VPV});
IV->replaceAllUsesWith(Add);
Add->setOperand(0, IV);

DenseMap<Value *, Value *> ToFrozen;
SmallVector<Instruction *> InstsToMove;
for (VPRecipeBase &R : drop_begin(Header->phis())) {
// Ensure that the start values for all header phi recipes are updated before
// vectorizing the epilogue loop.
for (VPRecipeBase &R : Header->phis()) {
Value *ResumeV = nullptr;
// TODO: Move setting of resume values to prepareToExecute.
if (auto *ReductionPhi = dyn_cast<VPReductionPHIRecipe>(&R)) {
Expand Down
71 changes: 58 additions & 13 deletions llvm/lib/Transforms/Vectorize/VPlan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,10 +768,18 @@ static std::pair<VPBlockBase *, VPBlockBase *> cloneFrom(VPBlockBase *Entry) {

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

if (CanIVInfo.CanIV) {
NewRegion->CanIVInfo.CanIV = new VPRegionValue();
NewRegion->CanIVInfo.Ty = CanIVInfo.Ty;
NewRegion->CanIVInfo.HasNUW = CanIVInfo.HasNUW;
NewRegion->CanIVInfo.DL = CanIVInfo.DL;
}

return NewRegion;
}

Expand Down Expand Up @@ -856,6 +864,11 @@ void VPRegionBlock::print(raw_ostream &O, const Twine &Indent,
VPSlotTracker &SlotTracker) const {
O << Indent << (isReplicator() ? "<xVFxUF> " : "<x1> ") << getName() << ": {";
auto NewIndent = Indent + " ";
if (auto *CanIV = getCanonicalIV()) {
O << '\n';
CanIV->print(O, SlotTracker);
O << '\n';
}
for (auto *BlockBase : vp_depth_first_shallow(Entry)) {
O << '\n';
BlockBase->print(O, NewIndent, SlotTracker);
Expand All @@ -868,18 +881,37 @@ void VPRegionBlock::print(raw_ostream &O, const Twine &Indent,

void VPRegionBlock::dissolveToCFGLoop() {
auto *Header = cast<VPBasicBlock>(getEntry());
if (auto *CanIV = dyn_cast<VPCanonicalIVPHIRecipe>(&Header->front())) {
assert(this == getPlan()->getVectorLoopRegion() &&
"Canonical IV must be in the entry of the top-level loop region");
auto *ScalarR = VPBuilder(CanIV).createScalarPhi(
{CanIV->getStartValue(), CanIV->getBackedgeValue()},
CanIV->getDebugLoc(), "index");
auto *ExitingLatch = cast<VPBasicBlock>(getExiting());
VPValue *CanIV = getCanonicalIV();
if (CanIV && CanIV->getNumUsers() > 0) {
auto *ExitingTerm = ExitingLatch->getTerminator();
VPInstruction *CanIVInc = nullptr;
// Check if there's a canonical IV increment via an existing terminator.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps CanIVInfo could cache CanIVInc?
Should CanIVInc also be a VPRegionValue?

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 sure if caching would work, as it may be removed w/o uses, leaving a dangling reference. Making it a region value would solve that, but best done separately?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure; perhaps worth delegating a getCanIVInc() to VPRegionBlock even if/while implemented by looking up the latch terminator, possibly as a step towards region value.

if (match(ExitingTerm,
m_BranchOnCount(m_VPInstruction(CanIVInc), m_VPValue()))) {
assert(match(CanIVInc,
m_Add(m_CombineOr(m_Specific(CanIV),
m_Add(m_Specific(CanIV), m_LiveIn())),
m_VPValue())) &&
"invalid existing IV increment");
}
Comment on lines +890 to +897
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems a bit odd to have only an assert under an if. Perhaps worth a comment, as this alternative seems even worse:

Suggested change
if (match(ExitingTerm,
m_BranchOnCount(m_VPInstruction(CanIVInc), m_VPValue()))) {
assert(match(CanIVInc,
m_Add(m_CombineOr(m_Specific(CanIV),
m_Add(m_Specific(CanIV), m_LiveIn())),
m_VPValue())) &&
"invalid existing IV increment");
}
[[maybe_unused]] bool BranchOnIVInc = match(ExitingTerm,
m_BranchOnCount(m_VPInstruction(CanIVInc), m_VPValue()));
assert(!BranchOnIVInc || match(CanIVInc,
m_Add(m_CombineOr(m_Specific(CanIV),
m_Add(m_Specific(CanIV), m_LiveIn())),
m_VPValue())) &&
"invalid existing IV increment");

VPlan &Plan = *getPlan();
if (!CanIVInc) {
CanIVInc = VPBuilder(ExitingTerm)
.createOverflowingOp(
Instruction::Add, {CanIV, &Plan.getVFxUF()},
{CanIVInfo.HasNUW, false}, CanIVInfo.DL, "index.next");
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
{CanIVInfo.HasNUW, false}, CanIVInfo.DL, "index.next");
{CanIVInfo.HasNUW, /* HasNSW */ false}, CanIVInfo.DL, "index.next");

}
auto *ScalarR =
VPBuilder(Header, Header->begin())
.createScalarPhi(
{Plan.getOrAddLiveIn(ConstantInt::get(CanIVInfo.Ty, 0)),
CanIVInc},
CanIVInfo.DL, "index");
CanIV->replaceAllUsesWith(ScalarR);
CanIV->eraseFromParent();
}

VPBlockBase *Preheader = getSinglePredecessor();
auto *ExitingLatch = cast<VPBasicBlock>(getExiting());
VPBlockBase *Middle = getSingleSuccessor();
VPBlockUtils::disconnectBlocks(Preheader, this);
VPBlockUtils::disconnectBlocks(this, Middle);
Expand Down Expand Up @@ -916,7 +948,10 @@ VPlan::~VPlan() {
for (unsigned I = 0, E = R.getNumOperands(); I != E; I++)
R.setOperand(I, &DummyValue);
}
} else if (auto *CanIV = cast<VPRegionBlock>(VPB)->getCanonicalIV()) {
CanIV->replaceAllUsesWith(&DummyValue);
}

delete VPB;
}
for (VPValue *VPV : getLiveIns())
Expand Down Expand Up @@ -1224,6 +1259,11 @@ VPlan *VPlan::duplicate() {
// else NewTripCount will be created and inserted into Old2NewVPValues when
// TripCount is cloned. In any case NewPlan->TripCount is updated below.

if (auto *LoopRegion = getVectorLoopRegion()) {
Old2NewVPValues[LoopRegion->getCanonicalIV()] =
NewPlan->getVectorLoopRegion()->getCanonicalIV();
}
Comment on lines +1262 to +1265
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 (auto *LoopRegion = getVectorLoopRegion()) {
Old2NewVPValues[LoopRegion->getCanonicalIV()] =
NewPlan->getVectorLoopRegion()->getCanonicalIV();
}
if (auto *CanIV = getCanonicalIV())
Old2NewVPValues[CanIV] = NewPlan->getCanonicalIV();

?


remapOperands(Entry, NewEntry, Old2NewVPValues);

// Initialize remaining fields of cloned VPlan.
Expand Down Expand Up @@ -1404,6 +1444,8 @@ void VPlanPrinter::dumpRegion(const VPRegionBlock *Region) {
/// Returns true if there is a vector loop region and \p VPV is defined in a
/// loop region.
static bool isDefinedInsideLoopRegions(const VPValue *VPV) {
if (isa<VPRegionValue>(VPV))
return true;
const VPRecipeBase *DefR = VPV->getDefiningRecipe();
return DefR && (!DefR->getParent()->getPlan()->getVectorLoopRegion() ||
DefR->getParent()->getEnclosingLoopRegion());
Expand Down Expand Up @@ -1513,9 +1555,12 @@ void VPSlotTracker::assignNames(const VPlan &Plan) {

ReversePostOrderTraversal<VPBlockDeepTraversalWrapper<const VPBlockBase *>>
RPOT(VPBlockDeepTraversalWrapper<const VPBlockBase *>(Plan.getEntry()));
for (const VPBasicBlock *VPBB :
VPBlockUtils::blocksOnly<const VPBasicBlock>(RPOT))
assignNames(VPBB);
for (const VPBlockBase *VPB : RPOT) {
if (auto *VPBB = dyn_cast<VPBasicBlock>(VPB))
assignNames(VPBB);
else if (auto *CanIV = cast<VPRegionBlock>(VPB)->getCanonicalIV())
assignName(CanIV);
}
}

void VPSlotTracker::assignNames(const VPBasicBlock *VPBB) {
Expand Down
Loading
Loading