-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[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
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
|
@@ -1242,12 +1242,24 @@ struct LLVM_ABI_FOR_TEST VPPhi : public VPInstruction, public VPPhiAccessors { | |
: VPInstruction(Instruction::PHI, Operands, DL, Name) {} | ||
|
||
static inline bool classof(const VPUser *U) { | ||
auto *R = dyn_cast<VPInstruction>(U); | ||
return R && R->getOpcode() == Instruction::PHI; | ||
auto *VPI = dyn_cast<VPInstruction>(U); | ||
return VPI && VPI->getOpcode() == Instruction::PHI; | ||
} | ||
|
||
static inline bool classof(const VPValue *V) { | ||
auto *VPI = dyn_cast<VPInstruction>(V); | ||
return VPI && VPI->getOpcode() == Instruction::PHI; | ||
Comment on lines
+1250
to
+1251
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. Also holds for above classof, for consistency. 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 also clean up, thanks |
||
} | ||
|
||
static inline bool classof(const VPSingleDefRecipe *SDR) { | ||
auto *VPI = dyn_cast<VPInstruction>(SDR); | ||
return VPI && VPI->getOpcode() == Instruction::PHI; | ||
} | ||
|
||
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))); | ||
} | ||
} | ||
|
@@ -204,11 +202,11 @@ void PlainCFGBuilder::createVPInstructionsForVPBB(VPBasicBlock *VPBB, | |
|
||
VPSingleDefRecipe *NewR; | ||
if (auto *Phi = dyn_cast<PHINode>(Inst)) { | ||
// Phi node's operands may have not been visited at this point. We create | ||
// Phi node's operands may not have been visited at this point. We create | ||
// 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()