-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[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
Changes from all commits
b357af4
9113022
c61233f
a73a137
90b2d4d
f768b97
d6d6bb5
8dfd569
595b057
a2b4ebb
46780ab
54981bf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
} | ||
|
||
/// Returns the incoming block with index \p Idx. | ||
const VPBasicBlock *getIncomingBlock(unsigned Idx) const; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It needs access to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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. | ||
lukel97 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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. | ||
|
@@ -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 | ||
|
@@ -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); | ||
|
@@ -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. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3686,27 +3686,6 @@ void VPReductionPHIRecipe::print(raw_ostream &O, const Twine &Indent, | |
} | ||
#endif | ||
|
||
VPBasicBlock *VPWidenPHIRecipe::getIncomingBlock(unsigned I) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Better keep delegating the implementation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now providing a specialized definition of |
||
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."); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
VPPhiAccessors
having bothgetIncomingBlock(Idx)
andgetIncomingValue(Idx)
be pure virtual, and have another interimVPSingleDefPhiRecipe
inherit from bothVPSingleDefRecipe
andVPPhiAccessors
take care of implementinggetIncomingValue(Idx)
for all (singleDef) phi recipes, instead ofgetAsRecipe()
subclass casting. Are all recipes candidate of inheritance singleDef?There was a problem hiding this comment.
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 fromVPSingleDefRecipe
, but inheriting fromVPSinglePhiDefRecipe
would not be approriate, hence the trait/mix-in. Down the road, we could also support casting any recipe that supports it toVPPhiAccessors
, e.g. for verifying all phi-like nodes that implement the trait.Alternatively we could manually add definitions of
getIncomingBlock
andgetIncomingValue
to all relevant classes?There was a problem hiding this comment.
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 toVPWidenPHIRecipe
(who could useVPSingleDefPhiRecipe
with other potential partners) andVPIRPhi
?If
VPIRPhi
inherits directly fromVPPhiAccessors
, 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
andgetIncomingValue
by the mix-in, as done here, or neither (and have both defined by all derived classes instead).There was a problem hiding this comment.
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
andgetIncomingValue
thereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another one is VPPredInstPhi
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?