-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[VPlan] Add exit phi operands during initial construction (NFC). #136455
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 5 commits
e931309
a854ca7
7052337
26d7348
d0081f7
ea95862
a900c63
5c37c32
412b278
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 |
---|---|---|
|
@@ -9368,8 +9368,7 @@ static void addScalarResumePhis(VPRecipeBuilder &Builder, VPlan &Plan, | |
} | ||
} | ||
|
||
// Collect VPIRInstructions for phis in the exit blocks that are modeled | ||
// in VPlan and add the exiting VPValue as operand. | ||
// Collect VPIRInstructions for phis in the exit block from the latch only. | ||
static SetVector<VPIRInstruction *> | ||
collectUsersInExitBlocks(Loop *OrigLoop, VPRecipeBuilder &Builder, | ||
VPlan &Plan) { | ||
|
@@ -9388,11 +9387,8 @@ collectUsersInExitBlocks(Loop *OrigLoop, VPRecipeBuilder &Builder, | |
continue; | ||
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: can R iterate over ExitVPBB->phis() 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 in 8c83355, thanks 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: is this early continue needed:
given that unreachable exit blocks have been emptied of their all their recipes - including phi ones? 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. Removed in e268f71, thanks 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: loop-unswitch this condition
rather than check it for all phi's? 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, thanks |
||
} | ||
|
||
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. Above "and add the exiting VPValue as operand" is now obsolete. Worth noting that users of multiple (early) exits are excluded? 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. Done, aslo dropped .. modeled in VPlan, as all are now modeled in VPlan. |
||
PHINode &ExitPhi = ExitIRI->getIRPhi(); | ||
BasicBlock *ExitingBB = OrigLoop->getLoopLatch(); | ||
Value *IncomingValue = ExitPhi.getIncomingValueForBlock(ExitingBB); | ||
VPValue *V = Builder.getVPValueOrAddLiveIn(IncomingValue); | ||
ExitIRI->addOperand(V); | ||
assert(ExitIRI->getNumOperands() == 1 && "must have a single operand"); | ||
VPValue *V = ExitIRI->getOperand(0); | ||
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. Worth asserting that ExitIRI has a single operand? Expected to match its single middle-block predecessor. 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. Done, thanks |
||
if (V->isLiveIn()) | ||
continue; | ||
assert(V->getDefiningRecipe()->getParent()->getEnclosingLoopRegion() && | ||
|
@@ -9421,7 +9417,7 @@ addUsersInExitBlocks(VPlan &Plan, | |
ExitIRI->getParent()->getSinglePredecessor() == MiddleVPBB && | ||
"exit values from early exits must be fixed when branch to " | ||
"early-exit is added"); | ||
ExitIRI->extractLastLaneOfOperand(B); | ||
ExitIRI->extractLastLaneOfFirstOperand(B); | ||
Comment on lines
9420
to
+9423
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: |
||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -352,6 +352,20 @@ std::unique_ptr<VPlan> PlainCFGBuilder::buildPlainCFG( | |||||
Plan->getEntry()->setOneSuccessor(getOrCreateVPBB(TheLoop->getHeader())); | ||||||
Plan->getEntry()->setPlan(&*Plan); | ||||||
|
||||||
// Fix VPlan loop-closed-ssa exit phi's by add incoming operands to the | ||||||
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
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. Done thanks |
||||||
// VPIRInstructions wrapping them. | ||||||
for (auto *EB : Plan->getExitBlocks()) { | ||||||
for (VPRecipeBase &R : EB->phis()) { | ||||||
auto *PhiR = cast<VPIRPhi>(&R); | ||||||
PHINode &Phi = PhiR->getIRPhi(); | ||||||
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. assert PhiR is still w/o any operand? 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. Done thanks |
||||||
assert(PhiR->getNumOperands() == 0 && | ||||||
"no phi operands should be added yet"); | ||||||
for (BasicBlock *Pred : predecessors(EB->getIRBasicBlock())) | ||||||
PhiR->addOperand( | ||||||
getOrCreateVPOperand(Phi.getIncomingValueForBlock(Pred))); | ||||||
Comment on lines
+366
to
+368
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. Order of operands set here corresponds to the order of predecessors of underlying IRBB, before EB VPBB has predecessors. This inconsistency requires attention later, when these predecessors are added, possibly in a different order. May be worth leaving a note. 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. Done, thanks |
||||||
} | ||||||
} | ||||||
|
||||||
for (const auto &[IRBB, VPB] : BB2VPBB) | ||||||
VPB2IRBB[VPB] = IRBB; | ||||||
|
||||||
|
@@ -464,6 +478,12 @@ void VPlanTransforms::createLoopRegions(VPlan &Plan, Type *InductionTy, | |||||
VPBlockUtils::connectBlocks(ScalarPH, Plan.getScalarHeader()); | ||||||
if (!RequiresScalarEpilogueCheck) { | ||||||
VPBlockUtils::connectBlocks(MiddleVPBB, ScalarPH); | ||||||
// The exit blocks are unreachable, remove their recipes to make sure no | ||||||
// users remain that may pessimize transforms. | ||||||
for (auto *EB : Plan.getExitBlocks()) { | ||||||
for (VPRecipeBase &R : make_early_inc_range(*EB)) | ||||||
R.eraseFromParent(); | ||||||
} | ||||||
Comment on lines
+495
to
+500
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. Deserves mentioning in createLoopRegions()'s documentation? Which indeed does much more than create loop regions. 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. Would it be best to separate region creation from other 'skeleton additions' separately instead? 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. Seems so - better build the complete skeleton CFG first, then convert it to HCFG by converting its loops into regions. 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 separately, thanks! 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. Should be taken care of in d2ce88a, thanks |
||||||
return; | ||||||
} | ||||||
|
||||||
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. The case
is handle by early return above. Better place the explanation earlier, and replace "Do nothing" with "Empty the unreachable exit blocks of their recipes". The connection from scalar loop to exit blocks is (currently) outside of VPlan's scope. 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. Done, thanks. Also changed 2->1 as this is the first handled case. |
||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -1137,10 +1137,10 @@ InstructionCost VPIRInstruction::computeCost(ElementCount VF, | |||||||||||||
return 0; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
void VPIRInstruction::extractLastLaneOfOperand(VPBuilder &Builder) { | ||||||||||||||
void VPIRInstruction::extractLastLaneOfFirstOperand(VPBuilder &Builder) { | ||||||||||||||
assert(isa<PHINode>(getInstruction()) && | ||||||||||||||
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 this be implemented for VPIRPhi instead? 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 update separately, thanks |
||||||||||||||
"can only add exiting operands to phi nodes"); | ||||||||||||||
assert(getNumOperands() == 1 && "must have a single operand"); | ||||||||||||||
"can only update exiting operands to phi nodes"); | ||||||||||||||
assert(getNumOperands() > 0 && "must have at least one operand"); | ||||||||||||||
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. And yet we continue to extract the first operand, only? extractLastLaneOf[First]Operand() 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, updated the name, thanks |
||||||||||||||
VPValue *Exiting = getOperand(0); | ||||||||||||||
if (!Exiting->isLiveIn()) { | ||||||||||||||
Comment on lines
1144
to
1145
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
? 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 adjust separately, thanks 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. done in 71f2c1e |
||||||||||||||
LLVMContext &Ctx = getInstruction().getContext(); | ||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -2505,35 +2505,43 @@ void VPlanTransforms::handleUncountableEarlyExit( | |||||||||||||||||
VPBuilder EarlyExitB(VectorEarlyExitVPBB); | ||||||||||||||||||
for (VPRecipeBase &R : VPEarlyExitBlock->phis()) { | ||||||||||||||||||
auto *ExitIRI = cast<VPIRPhi>(&R); | ||||||||||||||||||
PHINode &ExitPhi = ExitIRI->getIRPhi(); | ||||||||||||||||||
VPValue *IncomingFromEarlyExit = RecipeBuilder.getVPValueOrAddLiveIn( | ||||||||||||||||||
ExitPhi.getIncomingValueForBlock(UncountableExitingBlock)); | ||||||||||||||||||
|
||||||||||||||||||
// By default, assume early exit operand is first, e.g., when the two exit | ||||||||||||||||||
// blocks are distinct - VPEarlyExitBlock has a single predecessor. | ||||||||||||||||||
unsigned EarlyExitIdx = 0; | ||||||||||||||||||
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
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. Added, thanks 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. Hmm, perhaps its better instead to set EarlyExitIdx to be the last operand, consistently? If VPEarlyExitBlock has two predecessors, they are already ordered such that early exit is second, by construction. But its underlying IRBB may have its predecessors ordered the other way around, and it is this order which corresponds to the order of operands of VPEarlyExitBlock's phi recipes. Therefore, if early exit is the first predecessor of the underlying IRBB, we swap the operands of phi recipes, bringing them to match VPEarlyExitBlock's predecessor order with early exit being last (second). 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. On second thought, could we agree on
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. Done, thanks |
||||||||||||||||||
if (OrigLoop->getUniqueExitBlock()) { | ||||||||||||||||||
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: better ask instead if VPEarlyExitBlock has two predecessors? One (the last) was added above - VectorEarlyExitVPBB. This should match the case where OrigLoop has a unique exit block - which would be aka VPEarlyExitBlock, or rather its underlying EarlyExitIRBB.
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. Will check separately, thanks 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. It's more consistent with following explanation. |
||||||||||||||||||
// The incoming values currently correspond to the original IR | ||||||||||||||||||
// predecessors. After the transform, the first incoming value is coming | ||||||||||||||||||
// from the original loop latch, while the second operand is from the | ||||||||||||||||||
// early exit. Swap the phi operands, if the first predecessor in the | ||||||||||||||||||
// original IR is not the loop latch. | ||||||||||||||||||
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. Still a seems a bit confusing. Would something like this read clearer: If VPEarlyExitBlock has two predecessors, they are already ordered such that early exit is second (and latch exit is first), by construction. But its underlying IRBB (EarlyExitIRBB) may have its predecessors ordered the other way around, and it is the order of the latter which corresponds to the order of operands of VPEarlyExitBlock's phi recipes. Therefore, if early exit (UncountableExitingBlock) is the first predecessor of EarlyExitIRBB, we swap the operands of phi recipes, thereby bringing them to match VPEarlyExitBlock's predecessor order, with early exit being last (second). Otherwise they already match. 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. Adjusted, thanks |
||||||||||||||||||
if (*pred_begin(VPEarlyExitBlock->getIRBasicBlock()) != | ||||||||||||||||||
OrigLoop->getLoopLatch()) | ||||||||||||||||||
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. Is this clearer?
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. Done thanks |
||||||||||||||||||
ExitIRI->swapOperands(); | ||||||||||||||||||
|
||||||||||||||||||
EarlyExitIdx = 1; | ||||||||||||||||||
// If there's a unique exit block, VPEarlyExitBlock has 2 predecessors | ||||||||||||||||||
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
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. Done thanks |
||||||||||||||||||
// (MiddleVPBB and NewMiddle). Add the incoming value from MiddleVPBB | ||||||||||||||||||
// which is coming from the original latch. | ||||||||||||||||||
VPValue *IncomingFromLatch = RecipeBuilder.getVPValueOrAddLiveIn( | ||||||||||||||||||
ExitPhi.getIncomingValueForBlock(OrigLoop->getLoopLatch())); | ||||||||||||||||||
ExitIRI->addOperand(IncomingFromLatch); | ||||||||||||||||||
ExitIRI->extractLastLaneOfOperand(MiddleBuilder); | ||||||||||||||||||
// (MiddleVPBB and NewMiddle). Extract the last lane of the incoming value | ||||||||||||||||||
// from MiddleVPBB which is coming from the original latch. | ||||||||||||||||||
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
(The other immediate predecessor, which corresponds to early exit, is VectorEarlyExitVPBB, rather than NewMiddle). 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. Done thanks |
||||||||||||||||||
ExitIRI->extractLastLaneOfFirstOperand(MiddleBuilder); | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
VPValue *IncomingFromEarlyExit = ExitIRI->getOperand(EarlyExitIdx); | ||||||||||||||||||
auto IsVector = [](ElementCount VF) { return VF.isVector(); }; | ||||||||||||||||||
// When the VFs are vectors, need to add `extract` to get the incoming value | ||||||||||||||||||
// from early exit. When the range contains scalar VF, limit the range to | ||||||||||||||||||
// scalar VF to prevent mis-compilation for the range containing both scalar | ||||||||||||||||||
// and vector VFs. | ||||||||||||||||||
if (!IncomingFromEarlyExit->isLiveIn() && | ||||||||||||||||||
LoopVectorizationPlanner::getDecisionAndClampRange(IsVector, Range)) { | ||||||||||||||||||
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: clamping range inside VPlanTransform? Limiting the range to scalar VF - is another VPlan constructed for the vector (sub)range? 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. Yes, this is done early on when we are clamping the loop range for other reasons as well. 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) Still puzzled about clamping the range at this stage, when all VPlans were already created following range clampings. Rather than say asserting that the range contains either scalar or vector VF's but not both, and introduce extracts if it's the latter. Extracts added above for latch exit need not check vector VF's? 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. the transform isn't run when all VPlans have been created, but in 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. Ahh, very well, thanks. (Independent) May be good to indicate somehow transforms that may clamp the range - that belong to tryToBuildVPlanWithVPRecipes stage, and prevent them from operating afterwards, perhaps by disabling range clamping afterwards? 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. The range should only be available in |
||||||||||||||||||
// Update the incoming value from the early exit. | ||||||||||||||||||
VPValue *FirstActiveLane = EarlyExitB.createNaryOp( | ||||||||||||||||||
VPInstruction::FirstActiveLane, {EarlyExitTakenCond}, nullptr, | ||||||||||||||||||
"first.active.lane"); | ||||||||||||||||||
IncomingFromEarlyExit = EarlyExitB.createNaryOp( | ||||||||||||||||||
Instruction::ExtractElement, {IncomingFromEarlyExit, FirstActiveLane}, | ||||||||||||||||||
nullptr, "early.exit.value"); | ||||||||||||||||||
ExitIRI->setOperand(EarlyExitIdx, IncomingFromEarlyExit); | ||||||||||||||||||
} | ||||||||||||||||||
ExitIRI->addOperand(IncomingFromEarlyExit); | ||||||||||||||||||
} | ||||||||||||||||||
MiddleBuilder.createNaryOp(VPInstruction::BranchOnCond, {IsEarlyExitTaken}); | ||||||||||||||||||
|
||||||||||||||||||
|
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.
Updated name, thanks