Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 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->getNumIncomingValues(); ++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
20 changes: 20 additions & 0 deletions llvm/lib/Transforms/Vectorize/VPlan.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,26 @@ 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 &&
"when placed in an entry block, only 2 incoming blocks are available");
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
"when placed in an entry block, only 2 incoming blocks are available");
"loop region has a single predecessor (preheader), its entry block has 2 incoming blocks");

Perhaps better have the verifier assert loop regions have a single predecessor, than here.

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 the comment, will adjust the verifier separately, thanks!


// 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;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: redundant?

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped, thanks

return Pred->getExitingBasicBlock();
}

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

/// Helper type to provide functions to access incoming values and blocks for
/// phi-like recipes. RecipeTy must be a sub-class of VPRecipeBase.
template <typename RecipeTy> class VPPhiAccessors {
/// Return a VPRecipeBase* to the current object.
const VPRecipeBase *getAsRecipe() const {
return static_cast<const RecipeTy *>(this);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the templating really needed, can getAsRecipe() simply down cast this to VPRecipeBase?

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 it seems so, problem is that w/o the template there's no inheritance relation here, and static_casts are rejected (Curiously Recurring Template Pattern). For some reason, we need to cast exactly to the type.

But the template parameter may cause problems in the future, so I updated getAsRecipe to be pure virtual to be implemented by the derived classes. The single implementation could also be used for other trait classes in the future. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use case enabled by getting rid of the template argument is supporting dyn_cast from VPRecipeBase -> VPPhiAccessors, as used in #124838 ( https://github.com/llvm/llvm-project/pull/124838/files#diff-a69094b5fcfce6b2bf9e957e2ac7011e5492e81c885129506c90874375e621fbR210) by implementing CastInfo<VPPhiAccessors, const VPRecipeBase *>

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, would having getAsRecipe() do a dyn_cast to VPRecipeBase be ok? Possibly followed by an assert...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we could dyn_cast from VPPhiAccessors to VPRecipeBase because there's no directy relationship between them (and the this pointer for the VPPhiAccessors (sub-)object may not be the same as the VPRecipeBase (base-) object. The other way works, because we can down-cast to the concrete types inheriting from VPPhiAccessors).

}

public:
/// 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 {
return getAsRecipe()->getParent()->getCFGPredecessor(Idx);
}

unsigned getNumIncomingValues() const {
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
unsigned getNumIncomingValues() const {
/// Returns the number of incoming values, also number of incoming blocks.
unsigned getNumIncoming() const {

its both Values and Blocks?

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

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,7 +1993,8 @@ 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<VPWidenPHIRecipe> {
/// Name to use for the generated IR instruction for the widened phi.
std::string Name;

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 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