-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[VPlan] Use scalar VPPhi instead of VPWidenPHIRecipe in createPlainCFG. #150847
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 1 commit
b2940a8
051bf6f
9d58f06
62ce5ea
3f2f9f6
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 | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1243,8 +1243,20 @@ struct LLVM_ABI_FOR_TEST VPPhi : public VPInstruction, public VPPhiAccessors { | |||||||||||||
return R && R->getOpcode() == Instruction::PHI; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
static inline bool classof(const VPValue *V) { | ||||||||||||||
auto *R = dyn_cast<VPInstruction>(V); | ||||||||||||||
return R && R->getOpcode() == Instruction::PHI; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
static inline bool classof(const VPSingleDefRecipe *R) { | ||||||||||||||
auto *VPI = dyn_cast<VPInstruction>(R); | ||||||||||||||
return VPI && VPI->getOpcode() == Instruction::PHI; | ||||||||||||||
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.
Suggested change
consistency w/ the above. 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. Updated to use VPI consistently, thanks |
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
VPPhi *clone() override { | ||||||||||||||
return new VPPhi(operands(), getDebugLoc(), getName()); | ||||||||||||||
auto *PhiR = new VPPhi(operands(), getDebugLoc(), getName()); | ||||||||||||||
PhiR->setUnderlyingValue(getUnderlyingValue()); | ||||||||||||||
return PhiR; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
void execute(VPTransformState &State) override; | ||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -91,17 +91,15 @@ void PlainCFGBuilder::fixHeaderPhis() { | |||||
for (auto *Phi : PhisToFix) { | ||||||
assert(IRDef2VPValue.count(Phi) && "Missing VPInstruction for PHINode."); | ||||||
VPValue *VPVal = IRDef2VPValue[Phi]; | ||||||
assert(isa<VPWidenPHIRecipe>(VPVal) && | ||||||
"Expected WidenPHIRecipe for phi node."); | ||||||
auto *VPPhi = cast<VPWidenPHIRecipe>(VPVal); | ||||||
assert(VPPhi->getNumOperands() == 0 && | ||||||
"Expected VPInstruction with no operands."); | ||||||
assert(isa<VPPhi>(VPVal) && "Expected VPPhi for phi node."); | ||||||
auto *PhiR = cast<VPPhi>(VPVal); | ||||||
assert(PhiR->getNumOperands() == 0 && "Expected VPPhi with no operands."); | ||||||
assert(isHeaderBB(Phi->getParent(), LI->getLoopFor(Phi->getParent())) && | ||||||
"Expected Phi in header block."); | ||||||
assert(Phi->getNumOperands() == 2 && | ||||||
"header phi must have exactly 2 operands"); | ||||||
for (BasicBlock *Pred : predecessors(Phi->getParent())) | ||||||
VPPhi->addOperand( | ||||||
PhiR->addOperand( | ||||||
getOrCreateVPOperand(Phi->getIncomingValueForBlock(Pred))); | ||||||
} | ||||||
} | ||||||
|
@@ -207,8 +205,8 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB, | |||||
// Phi node's operands may have not been visited at this point. We create | ||||||
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. independent nit:
Suggested change
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. Fixed thanks |
||||||
// an empty VPInstruction that we will fix once the whole plain CFG has | ||||||
// been built. | ||||||
NewR = new VPWidenPHIRecipe(Phi, nullptr, Phi->getDebugLoc(), "vec.phi"); | ||||||
VPBB->appendRecipe(NewR); | ||||||
NewR = VPIRBuilder.createScalarPhi({}, Phi->getDebugLoc(), "vec.phi"); | ||||||
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. Independent: should 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. For now it is scalar-phi only, but the distinction could go away in the later stages, if we decide to encode single-scalar/wide in for some recipes. 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, may be good to clarify/document the distinction between VPWidenPHIRecipe and VPPhi - the former holds for VF>1 and/or UF>1, the latter holds for scalar, VF=UF=1, currently? Would be good to resolve the discrepancy between createScalarPhi and VPPhi, one way or another, if they continue to be synonymous. 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. Will do. To clarify, VPPhi will also be used for scalar phis that remain scalar even with VF > 1 (at least when lowering for exexcute) |
||||||
NewR->setUnderlyingValue(Phi); | ||||||
if (isHeaderBB(Phi->getParent(), LI->getLoopFor(Phi->getParent()))) { | ||||||
// Header phis need to be fixed after the VPBB for the latch has been | ||||||
// created. | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,17 +63,20 @@ bool VPlanTransforms::tryToConvertVPInstructionsToVPRecipes( | |
Instruction *Inst = cast<Instruction>(VPV->getUnderlyingValue()); | ||
|
||
VPRecipeBase *NewRecipe = nullptr; | ||
if (auto *VPPhi = dyn_cast<VPWidenPHIRecipe>(&Ingredient)) { | ||
auto *Phi = cast<PHINode>(VPPhi->getUnderlyingValue()); | ||
if (auto *PhiR = dyn_cast<VPPhi>(&Ingredient)) { | ||
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. Independent: 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. Yep, will do separately, thanks |
||
auto *Phi = cast<PHINode>(PhiR->getUnderlyingValue()); | ||
const auto *II = GetIntOrFpInductionDescriptor(Phi); | ||
if (!II) | ||
continue; | ||
|
||
VPValue *Start = Plan->getOrAddLiveIn(II->getStartValue()); | ||
VPValue *Step = | ||
vputils::getOrCreateVPValueForSCEVExpr(*Plan, II->getStep(), SE); | ||
NewRecipe = new VPWidenIntOrFpInductionRecipe( | ||
Phi, Start, Step, &Plan->getVF(), *II, Ingredient.getDebugLoc()); | ||
if (!II) { | ||
NewRecipe = new VPWidenPHIRecipe(Phi, nullptr, PhiR->getDebugLoc()); | ||
for (VPValue *Op : PhiR->operands()) | ||
NewRecipe->addOperand(Op); | ||
} else { | ||
VPValue *Start = Plan->getOrAddLiveIn(II->getStartValue()); | ||
VPValue *Step = | ||
vputils::getOrCreateVPValueForSCEVExpr(*Plan, II->getStep(), SE); | ||
NewRecipe = new VPWidenIntOrFpInductionRecipe( | ||
Phi, Start, Step, &Plan->getVF(), *II, Ingredient.getDebugLoc()); | ||
} | ||
} else { | ||
assert(isa<VPInstruction>(&Ingredient) && | ||
"only VPInstructions expected here"); | ||
|
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.
?
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.
Yep, added an assert to the else branch using
!VPRecipeBase::isPhi()