-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[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 11 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,29 @@ 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; | ||||||||
|
Collaborator
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().
Contributor
Author
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
Collaborator
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. |
||||||||
|
|
||||||||
| unsigned getNumIncomingValues() const { | ||||||||
|
||||||||
| unsigned getNumIncomingValues() const { | |
| /// Returns the number of incoming values, also number of incoming blocks. | |
| unsigned getNumIncoming() const { |
its both Values and Blocks?
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.
Done thanks
lukel97 marked this conversation as resolved.
Show resolved
Hide resolved
| 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) { | ||
|
Collaborator
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
Contributor
Author
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
VPPhiAccessorshaving bothgetIncomingBlock(Idx)andgetIncomingValue(Idx)be pure virtual, and have another interimVPSingleDefPhiRecipeinherit from bothVPSingleDefRecipeandVPPhiAccessorstake 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
VPIRInstructioninherits fromVPSingleDefRecipe, but inheriting fromVPSinglePhiDefRecipewould 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
getIncomingBlockandgetIncomingValueto 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
VPPhiAccessorsin addition toVPWidenPHIRecipe(who could useVPSingleDefPhiRecipewith other potential partners) andVPIRPhi?If
VPIRPhiinherits 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
getIncomingBlockandgetIncomingValueby 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
getIncomingBlockandgetIncomingValuethereThere 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
getAsRecipepure-virtual to avoid the template argument + static cast. WDYT?