Skip to content

[VPlan] Add VPPhiAccessors to provide interface for phi recipes (NFC) #129388

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

Merged
merged 12 commits into from
May 4, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 2 additions & 2 deletions llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2883,9 +2883,9 @@ void InnerLoopVectorizer::fixNonInductionPHIs(VPTransformState &State) {
PHINode *NewPhi = cast<PHINode>(State.get(VPPhi));
// Make sure the builder has a valid insert point.
Builder.SetInsertPoint(NewPhi);
for (unsigned Idx = 0; Idx < VPPhi->getNumOperands(); ++Idx) {
for (unsigned Idx = 0; Idx < VPPhi->getNumIncoming(); ++Idx) {
VPValue *Inc = VPPhi->getIncomingValue(Idx);
VPBasicBlock *VPBB = VPPhi->getIncomingBlock(Idx);
const VPBasicBlock *VPBB = VPPhi->getIncomingBlock(Idx);
NewPhi->addIncoming(State.get(Inc), State.CFG.VPBB2IRBB[VPBB]);
}
}
Expand Down
19 changes: 19 additions & 0 deletions llvm/lib/Transforms/Vectorize/VPlan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,25 @@ InstructionCost VPBasicBlock::cost(ElementCount VF, VPCostContext &Ctx) {
return Cost;
}

const VPBasicBlock *VPBasicBlock::getCFGPredecessor(unsigned Idx) const {
const VPBlockBase *Pred = nullptr;
if (getNumPredecessors() > 0) {
Pred = getPredecessors()[Idx];
} else {
auto *Region = getParent();
assert(Region && !Region->isReplicator() && Region->getEntry() == this &&
"must be in the entry block of a non-replicate region");
assert(Idx < 2 && Region->getNumPredecessors() == 1 &&
"loop region has a single predecessor (preheader), its entry block "
"has 2 incoming blocks");

// Idx == 0 selects the predecessor of the region, Idx == 1 selects the
// region itself whose exiting block feeds the phi across the backedge.
Pred = Idx == 0 ? Region->getSinglePredecessor() : Region;
}
return Pred->getExitingBasicBlock();
}

InstructionCost VPRegionBlock::cost(ElementCount VF, VPCostContext &Ctx) {
if (!isReplicator()) {
InstructionCost Cost = 0;
Expand Down
44 changes: 37 additions & 7 deletions llvm/lib/Transforms/Vectorize/VPlan.h
Original file line number Diff line number Diff line change
Expand Up @@ -1166,6 +1166,28 @@ class VPIRInstruction : public VPRecipeBase {
void extractLastLaneOfFirstOperand(VPBuilder &Builder);
};

/// Helper type to provide functions to access incoming values and blocks for
/// phi-like recipes.
class VPPhiAccessors {
protected:
/// Return a VPRecipeBase* to the current object.
virtual const VPRecipeBase *getAsRecipe() const = 0;

public:
virtual ~VPPhiAccessors() = default;

/// Returns the incoming VPValue with index \p Idx.
VPValue *getIncomingValue(unsigned Idx) const {
return getAsRecipe()->getOperand(Idx);
}
Comment on lines +1180 to +1182
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 VPPhiAccessors having both getIncomingBlock(Idx) and getIncomingValue(Idx) be pure virtual, and have another interim VPSingleDefPhiRecipe inherit from both VPSingleDefRecipe and VPPhiAccessors take care of implementing getIncomingValue(Idx) for all (singleDef) phi recipes, instead of getAsRecipe() subclass casting. Are all recipes candidate of inheritance singleDef?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All recipes are single defs, but we now unfortunately have some recipes (e.g. VPIRPhi) where the base class VPIRInstruction inherits from VPSingleDefRecipe, but inheriting from VPSinglePhiDefRecipe would not be approriate, hence the trait/mix-in. Down the road, we could also support casting any recipe that supports it to VPPhiAccessors, e.g. for verifying all phi-like nodes that implement the trait.

Alternatively we could manually add definitions of getIncomingBlock and getIncomingValue to all relevant classes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, who are all the relevant classes - expected to inherit directly from VPPhiAccessors in addition to VPWidenPHIRecipe (who could use VPSingleDefPhiRecipe with other potential partners) and VPIRPhi?

If VPIRPhi inherits directly from VPPhiAccessors, could it implement getIncomingBlock based on the direct predecessors of its VPBasicBlock, as it is not used to represent header phi's of HCFG regions? I.e., assert it has direct predecessors.

In any case, good to implement both getIncomingBlock and getIncomingValue by the mix-in, as done here, or neither (and have both defined by all derived classes 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.

we need it for VPWidenPHIRecipe, VPHeaderPHIRecipe, VPIRPhi, VPEVLBasedPhi and VPPhi (scalar phis via VPInstruction, probably via a new specialization).

Define both getIncomingBlock and getIncomingValue there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another one is VPPredInstPhi

Copy link
Collaborator

Choose a reason for hiding this comment

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

VPWidenPHIRecipe, VPHeaderPHIRecipe (base class of VPEVLBasedPhi), and VPPredInstPhi all inherit from VPSingleDefRecipe. So could inherit from VPSingleDefPhiRecipe instead, which could take care of implementing these pure virtual methods for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the getAsRecipe pure-virtual to avoid the template argument + static cast. WDYT?


/// Returns the incoming block with index \p Idx.
const VPBasicBlock *getIncomingBlock(unsigned Idx) const;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Inline as well? Good to see its implementation next to that of getIncomingValue() and getNumIncoming().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It needs access to VPBasicBlock's definition, which isn't available here; could be resolved by moving VPBlock* definitions to a separate header possibly

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, probably better done separately, if at all.


/// Returns the number of incoming values, also number of incoming blocks.
unsigned getNumIncoming() const { return getAsRecipe()->getNumOperands(); }
};

/// An overlay for VPIRInstructions wrapping PHI nodes enabling convenient use
/// cast/dyn_cast/isa and execute() implementation. A single VPValue operand is
/// allowed, and it is used to add a new incoming value for the single
Expand Down Expand Up @@ -1969,10 +1991,13 @@ class VPWidenPointerInductionRecipe : public VPWidenInductionRecipe,
/// recipe is placed in an entry block to a (non-replicate) region, it must have
/// exactly 2 incoming values, the first from the predecessor of the region and
/// the second from the exiting block of the region.
class VPWidenPHIRecipe : public VPSingleDefRecipe {
class VPWidenPHIRecipe : public VPSingleDefRecipe, public VPPhiAccessors {
/// Name to use for the generated IR instruction for the widened phi.
std::string Name;

protected:
const VPRecipeBase *getAsRecipe() const override { return this; }

public:
/// Create a new VPWidenPHIRecipe for \p Phi with start value \p Start and
/// debug location \p DL.
Expand Down Expand Up @@ -2000,12 +2025,6 @@ class VPWidenPHIRecipe : public VPSingleDefRecipe {
void print(raw_ostream &O, const Twine &Indent,
VPSlotTracker &SlotTracker) const override;
#endif

/// Returns the \p I th incoming VPBasicBlock.
VPBasicBlock *getIncomingBlock(unsigned I);

/// Returns the \p I th incoming VPValue.
VPValue *getIncomingValue(unsigned I) { return getOperand(I); }
};

/// A recipe for handling first-order recurrence phis. The start value is the
Expand Down Expand Up @@ -3324,6 +3343,12 @@ class VPBasicBlock : public VPBlockBase {
/// the cloned recipes.
VPBasicBlock *clone() override;

/// Returns the predecessor block at index \p Idx with the predecessors as per
/// the corresponding plain CFG. If the block is an entry block to a region,
/// the first predecessor is the single predecessor of a region, and the
/// second predecessor is the exiting block of the region.
const VPBasicBlock *getCFGPredecessor(unsigned Idx) const;

protected:
/// Execute the recipes in the IR basic block \p BB.
void executeRecipes(VPTransformState *State, BasicBlock *BB);
Expand All @@ -3338,6 +3363,11 @@ class VPBasicBlock : public VPBlockBase {
BasicBlock *createEmptyBasicBlock(VPTransformState &State);
};

inline const VPBasicBlock *
VPPhiAccessors::getIncomingBlock(unsigned Idx) const {
return getAsRecipe()->getParent()->getCFGPredecessor(Idx);
}

/// A special type of VPBasicBlock that wraps an existing IR basic block.
/// Recipes of the block get added before the first non-phi instruction in the
/// wrapped block.
Expand Down
21 changes: 0 additions & 21 deletions llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3686,27 +3686,6 @@ void VPReductionPHIRecipe::print(raw_ostream &O, const Twine &Indent,
}
#endif

VPBasicBlock *VPWidenPHIRecipe::getIncomingBlock(unsigned I) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better keep delegating the implementation of getIncomingBlock(Idx) to recipes that inherit from VPPhiAccessors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now providing a specialized definition of VPPhiAccessors<VPWidenPHIRecipe>::getIncomingBlock, which is needed to instantiate the template for the type and use the generic getIncomingBlockForRecipe

VPBasicBlock *Parent = getParent();
VPBlockBase *Pred = nullptr;
if (Parent->getNumPredecessors() > 0) {
Pred = Parent->getPredecessors()[I];
} else {
auto *Region = Parent->getParent();
assert(Region && !Region->isReplicator() && Region->getEntry() == Parent &&
"must be in the entry block of a non-replicate region");
assert(
I < 2 && getNumOperands() == 2 &&
"when placed in an entry block, only 2 incoming blocks are available");

// I == 0 selects the predecessor of the region, I == 1 selects the region
// itself whose exiting block feeds the phi across the backedge.
Pred = I == 0 ? Region->getSinglePredecessor() : Region;
}

return Pred->getExitingBasicBlock();
}

void VPWidenPHIRecipe::execute(VPTransformState &State) {
assert(EnableVPlanNativePath &&
"Non-native vplans are not expected to have VPWidenPHIRecipes.");
Expand Down