[VPlan] Generalize collectUsersInExitBlocks for multiple exit bbs. #115066
Conversation
Generalize collectUsersInExitBlock to collecting exit users in multiple exit blocks. Exit blocks are leaf nodes in the VPlan (without successors) except the scalar header. Split off from llvm#112138
|
@llvm/pr-subscribers-vectorizers Author: Florian Hahn (fhahn) ChangesGeneralize collectUsersInExitBlock to collecting exit users in multiple exit blocks. Exit blocks are leaf nodes in the VPlan (without successors) except the scalar header. Split off in preparation for #112138 Full diff: https://github.com/llvm/llvm-project/pull/115066.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 35113c204b3c16..944896a75cc330 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -8819,47 +8819,51 @@ static void addScalarResumePhis(VPRecipeBuilder &Builder, VPlan &Plan) {
}
}
-// Collect VPIRInstructions for phis in the original exit block that are modeled
+// Collect VPIRInstructions for phis in the exit blocks that are modeled
// in VPlan and add the exiting VPValue as operand. Some exiting values are not
// modeled explicitly yet and won't be included. Those are un-truncated
// VPWidenIntOrFpInductionRecipe, VPWidenPointerInductionRecipe and induction
// increments.
-static SetVector<VPIRInstruction *> collectUsersInExitBlock(
+static SetVector<VPIRInstruction *> collectUsersInExitBlocks(
Loop *OrigLoop, VPRecipeBuilder &Builder, VPlan &Plan,
const MapVector<PHINode *, InductionDescriptor> &Inductions) {
- auto *MiddleVPBB = Plan.getMiddleBlock();
- // No edge from the middle block to the unique exit block has been inserted
- // and there is nothing to fix from vector loop; phis should have incoming
- // from scalar loop only.
- if (MiddleVPBB->getNumSuccessors() != 2)
- return {};
SetVector<VPIRInstruction *> ExitUsersToFix;
- VPBasicBlock *ExitVPBB = cast<VPIRBasicBlock>(MiddleVPBB->getSuccessors()[0]);
- BasicBlock *ExitingBB = OrigLoop->getExitingBlock();
- for (VPRecipeBase &R : *ExitVPBB) {
- auto *ExitIRI = dyn_cast<VPIRInstruction>(&R);
- if (!ExitIRI)
- continue;
- auto *ExitPhi = dyn_cast<PHINode>(&ExitIRI->getInstruction());
- if (!ExitPhi)
- break;
- Value *IncomingValue = ExitPhi->getIncomingValueForBlock(ExitingBB);
- VPValue *V = Builder.getVPValueOrAddLiveIn(IncomingValue);
- // Exit values for inductions are computed and updated outside of VPlan and
- // independent of induction recipes.
- // TODO: Compute induction exit values in VPlan.
- if ((isa<VPWidenIntOrFpInductionRecipe>(V) &&
- !cast<VPWidenIntOrFpInductionRecipe>(V)->getTruncInst()) ||
- isa<VPWidenPointerInductionRecipe>(V) ||
- (isa<Instruction>(IncomingValue) &&
- OrigLoop->contains(cast<Instruction>(IncomingValue)) &&
- any_of(IncomingValue->users(), [&Inductions](User *U) {
- auto *P = dyn_cast<PHINode>(U);
- return P && Inductions.contains(P);
- })))
+ for (VPBlockBase *VPB : vp_depth_first_shallow(
+ Plan.getVectorLoopRegion()->getSingleSuccessor())) {
+ if (VPB->getNumSuccessors() != 0 || VPB == Plan.getScalarHeader())
continue;
- ExitUsersToFix.insert(ExitIRI);
- ExitIRI->addOperand(V);
+ auto *ExitVPBB = cast<VPIRBasicBlock>(VPB);
+ BasicBlock *ExitBB = ExitVPBB->getIRBasicBlock();
+ BasicBlock *ExitingBB = find_singleton<BasicBlock>(
+ to_vector(predecessors(ExitBB)),
+ [OrigLoop](BasicBlock *Pred, bool AllowRepeats) {
+ return OrigLoop->contains(Pred) ? Pred : nullptr;
+ });
+ for (VPRecipeBase &R : *ExitVPBB) {
+ auto *ExitIRI = dyn_cast<VPIRInstruction>(&R);
+ if (!ExitIRI)
+ continue;
+ auto *ExitPhi = dyn_cast<PHINode>(&ExitIRI->getInstruction());
+ if (!ExitPhi)
+ break;
+ Value *IncomingValue = ExitPhi->getIncomingValueForBlock(ExitingBB);
+ VPValue *V = Builder.getVPValueOrAddLiveIn(IncomingValue);
+ // Exit values for inductions are computed and updated outside of VPlan
+ // and independent of induction recipes.
+ // TODO: Compute induction exit values in VPlan.
+ if ((isa<VPWidenIntOrFpInductionRecipe>(V) &&
+ !cast<VPWidenIntOrFpInductionRecipe>(V)->getTruncInst()) ||
+ isa<VPWidenPointerInductionRecipe>(V) ||
+ (isa<Instruction>(IncomingValue) &&
+ OrigLoop->contains(cast<Instruction>(IncomingValue)) &&
+ any_of(IncomingValue->users(), [&Inductions](User *U) {
+ auto *P = dyn_cast<PHINode>(U);
+ return P && Inductions.contains(P);
+ })))
+ continue;
+ ExitUsersToFix.insert(ExitIRI);
+ ExitIRI->addOperand(V);
+ }
}
return ExitUsersToFix;
}
@@ -8867,7 +8871,7 @@ static SetVector<VPIRInstruction *> collectUsersInExitBlock(
// Add exit values to \p Plan. Extracts are added for each entry in \p
// ExitUsersToFix if needed and their operands are updated.
static void
-addUsersInExitBlock(VPlan &Plan,
+addUsersInExitBlocks(VPlan &Plan,
const SetVector<VPIRInstruction *> &ExitUsersToFix) {
if (ExitUsersToFix.empty())
return;
@@ -9161,10 +9165,10 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
RecipeBuilder.fixHeaderPhis();
addScalarResumePhis(RecipeBuilder, *Plan);
- SetVector<VPIRInstruction *> ExitUsersToFix = collectUsersInExitBlock(
+ SetVector<VPIRInstruction *> ExitUsersToFix = collectUsersInExitBlocks(
OrigLoop, RecipeBuilder, *Plan, Legal->getInductionVars());
addExitUsersForFirstOrderRecurrences(*Plan, ExitUsersToFix);
- addUsersInExitBlock(*Plan, ExitUsersToFix);
+ addUsersInExitBlocks(*Plan, ExitUsersToFix);
// ---------------------------------------------------------------------------
// Transform initial VPlan: Apply previously taken decisions, in order, to
// bring the VPlan to its final state.
|
|
@llvm/pr-subscribers-llvm-transforms Author: Florian Hahn (fhahn) ChangesGeneralize collectUsersInExitBlock to collecting exit users in multiple exit blocks. Exit blocks are leaf nodes in the VPlan (without successors) except the scalar header. Split off in preparation for #112138 Full diff: https://github.com/llvm/llvm-project/pull/115066.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
index 35113c204b3c16..944896a75cc330 100644
--- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
+++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -8819,47 +8819,51 @@ static void addScalarResumePhis(VPRecipeBuilder &Builder, VPlan &Plan) {
}
}
-// Collect VPIRInstructions for phis in the original exit block that are modeled
+// Collect VPIRInstructions for phis in the exit blocks that are modeled
// in VPlan and add the exiting VPValue as operand. Some exiting values are not
// modeled explicitly yet and won't be included. Those are un-truncated
// VPWidenIntOrFpInductionRecipe, VPWidenPointerInductionRecipe and induction
// increments.
-static SetVector<VPIRInstruction *> collectUsersInExitBlock(
+static SetVector<VPIRInstruction *> collectUsersInExitBlocks(
Loop *OrigLoop, VPRecipeBuilder &Builder, VPlan &Plan,
const MapVector<PHINode *, InductionDescriptor> &Inductions) {
- auto *MiddleVPBB = Plan.getMiddleBlock();
- // No edge from the middle block to the unique exit block has been inserted
- // and there is nothing to fix from vector loop; phis should have incoming
- // from scalar loop only.
- if (MiddleVPBB->getNumSuccessors() != 2)
- return {};
SetVector<VPIRInstruction *> ExitUsersToFix;
- VPBasicBlock *ExitVPBB = cast<VPIRBasicBlock>(MiddleVPBB->getSuccessors()[0]);
- BasicBlock *ExitingBB = OrigLoop->getExitingBlock();
- for (VPRecipeBase &R : *ExitVPBB) {
- auto *ExitIRI = dyn_cast<VPIRInstruction>(&R);
- if (!ExitIRI)
- continue;
- auto *ExitPhi = dyn_cast<PHINode>(&ExitIRI->getInstruction());
- if (!ExitPhi)
- break;
- Value *IncomingValue = ExitPhi->getIncomingValueForBlock(ExitingBB);
- VPValue *V = Builder.getVPValueOrAddLiveIn(IncomingValue);
- // Exit values for inductions are computed and updated outside of VPlan and
- // independent of induction recipes.
- // TODO: Compute induction exit values in VPlan.
- if ((isa<VPWidenIntOrFpInductionRecipe>(V) &&
- !cast<VPWidenIntOrFpInductionRecipe>(V)->getTruncInst()) ||
- isa<VPWidenPointerInductionRecipe>(V) ||
- (isa<Instruction>(IncomingValue) &&
- OrigLoop->contains(cast<Instruction>(IncomingValue)) &&
- any_of(IncomingValue->users(), [&Inductions](User *U) {
- auto *P = dyn_cast<PHINode>(U);
- return P && Inductions.contains(P);
- })))
+ for (VPBlockBase *VPB : vp_depth_first_shallow(
+ Plan.getVectorLoopRegion()->getSingleSuccessor())) {
+ if (VPB->getNumSuccessors() != 0 || VPB == Plan.getScalarHeader())
continue;
- ExitUsersToFix.insert(ExitIRI);
- ExitIRI->addOperand(V);
+ auto *ExitVPBB = cast<VPIRBasicBlock>(VPB);
+ BasicBlock *ExitBB = ExitVPBB->getIRBasicBlock();
+ BasicBlock *ExitingBB = find_singleton<BasicBlock>(
+ to_vector(predecessors(ExitBB)),
+ [OrigLoop](BasicBlock *Pred, bool AllowRepeats) {
+ return OrigLoop->contains(Pred) ? Pred : nullptr;
+ });
+ for (VPRecipeBase &R : *ExitVPBB) {
+ auto *ExitIRI = dyn_cast<VPIRInstruction>(&R);
+ if (!ExitIRI)
+ continue;
+ auto *ExitPhi = dyn_cast<PHINode>(&ExitIRI->getInstruction());
+ if (!ExitPhi)
+ break;
+ Value *IncomingValue = ExitPhi->getIncomingValueForBlock(ExitingBB);
+ VPValue *V = Builder.getVPValueOrAddLiveIn(IncomingValue);
+ // Exit values for inductions are computed and updated outside of VPlan
+ // and independent of induction recipes.
+ // TODO: Compute induction exit values in VPlan.
+ if ((isa<VPWidenIntOrFpInductionRecipe>(V) &&
+ !cast<VPWidenIntOrFpInductionRecipe>(V)->getTruncInst()) ||
+ isa<VPWidenPointerInductionRecipe>(V) ||
+ (isa<Instruction>(IncomingValue) &&
+ OrigLoop->contains(cast<Instruction>(IncomingValue)) &&
+ any_of(IncomingValue->users(), [&Inductions](User *U) {
+ auto *P = dyn_cast<PHINode>(U);
+ return P && Inductions.contains(P);
+ })))
+ continue;
+ ExitUsersToFix.insert(ExitIRI);
+ ExitIRI->addOperand(V);
+ }
}
return ExitUsersToFix;
}
@@ -8867,7 +8871,7 @@ static SetVector<VPIRInstruction *> collectUsersInExitBlock(
// Add exit values to \p Plan. Extracts are added for each entry in \p
// ExitUsersToFix if needed and their operands are updated.
static void
-addUsersInExitBlock(VPlan &Plan,
+addUsersInExitBlocks(VPlan &Plan,
const SetVector<VPIRInstruction *> &ExitUsersToFix) {
if (ExitUsersToFix.empty())
return;
@@ -9161,10 +9165,10 @@ LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes(VFRange &Range) {
RecipeBuilder.fixHeaderPhis();
addScalarResumePhis(RecipeBuilder, *Plan);
- SetVector<VPIRInstruction *> ExitUsersToFix = collectUsersInExitBlock(
+ SetVector<VPIRInstruction *> ExitUsersToFix = collectUsersInExitBlocks(
OrigLoop, RecipeBuilder, *Plan, Legal->getInductionVars());
addExitUsersForFirstOrderRecurrences(*Plan, ExitUsersToFix);
- addUsersInExitBlock(*Plan, ExitUsersToFix);
+ addUsersInExitBlocks(*Plan, ExitUsersToFix);
// ---------------------------------------------------------------------------
// Transform initial VPlan: Apply previously taken decisions, in order, to
// bring the VPlan to its final state.
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Hi @fhahn, it doesn't look like #112138 has to deal with users of exit values, which this patch says it's in preparation for. Do you plan to expand the scope of PR #112138 to include more of PR #88385? I am very grateful for your help and feedback in trying to drive PR #88385 forward, but I was under the impression that PR #112138 was simply presenting an alternative basic skeleton for handling multiple loop region successors in VPlan, from which I could then continue to drive PR #88385 forwards. I'm not against doing it the way you've presented here, but the reason my patch separates them on different lists is because you need to calculate the final value in a different way to normal exits and so this PR would be functionally incorrect. You can't use ExtractFromEnd recipe because we cannot extract from the end of vector - we must extract from the lane that triggered the exit and you can see how I've done this in https://github.com/david-arm/llvm-project/tree/ee_autovec2. Also, I was planning with #88385 to put the early exit autovec support in incrementally in smaller patches to make it easier for reviewers, i.e.:
|
…ers-multiple-blocks
This is trying to avoid having to add extra code dealing with the exit phis following on the suggestions/comments in https://github.com/llvm/llvm-project/pull/112138/files#r1810491008. #112138 still only handles live-ins in exit phis, but in the latest version it doesn't have to update the VPIRInstructions; instead it rejects any VPlans that contains VPIRInstructions without operands (i.e. because they are live-out inductions) or any of their added operands aren't live-ins. Alternatively I could also restore the code from https://github.com/llvm/llvm-project/pull/112138/files#r1810491008, WDYT? |
IIRC we briefly talked about having support for phis that use only live-outs to test the whole structure more end-to-end, but another alternative would be to check the exit blocks for any VPIRInstruction instead of the current check around https://github.com/llvm/llvm-project/pull/112138/files#diff-da321d454a7246f8ae276bf1db2782bf26b5210b8133cb59e4d7fd45d0905decR9196 |
Hi @fhahn, thanks for the quick response! When I was looking at this patch yesterday PR #112138 hadn't been updated - I think there must have been an incorrect version pushed up because you'd addressed some comments but the code hadn't changed. So I was lacking some context about what how this patch fits in with that, but now I see your updated version and it makes more sense now! And I understand you're trying to limit the patch to only handling live-ins, which is fine. I'm currently reviewing your latest version of PR #112138 first, then I'll come back to this. |
| VPBlockUtils::blocksOnly<VPIRBasicBlock>( | ||
| vp_depth_first_shallow(getVectorLoopRegion()->getSingleSuccessor())), | ||
| [ScalarHeader](VPIRBasicBlock *VPIRBB) { | ||
| return VPIRBB != ScalarHeader && VPIRBB->getNumSuccessors() == 0; |
There was a problem hiding this comment.
Same comment as in the other PR - I think this can return different blocks depending upon whether you call this before or during VPlan::execute. Is this something we should worry about?
There was a problem hiding this comment.
It should always return the exit blocks currently in the VPlan, so if some transformation replaces/removes some of the exit blocks, the result would be different. But at the moment, the exit blocks should be fixed, we know them at time of construction and can wrap them in VPIRBasicBlocks and those shouldn't change (and if they change the transform would be responsible to make sure the exit phis are updated accordingly).
Is there anything particular in VPlan::execute you are thinking of? (We replace VPBasicBlocks with VPIRBasicBlocks in some cases if they are only created during skeleton creation, but that shouldn't be the case for the exit blocks)
There was a problem hiding this comment.
Currently isn't the middle block the single successor of the loop region? Perhaps I've misunderstood what VPBlockUtils::blocksOnly<VPIRBasicBlock> does but it looks like it may or may not include the middle block depending upon the timing. In VPlan::execute we replace the VPBasicBlock for the middle block with a VPIRBasicBlock.
There was a problem hiding this comment.
Ah, I think the reason why this works is because of the extra VPIRBB->getNumSuccessors() == 0 condition, since the middle block will always have at least one successor.
There was a problem hiding this comment.
The middle-block may or may not be included by VPBlockUtils::blocksOnly<VPIRBasicBlock>, but the middle block will never be considered as an exit block, as it has successors.
There was a problem hiding this comment.
Yeah, thanks for explaining! I realised that around the same time that you replied. It just took me a while to figure it out. :)
| @@ -3837,6 +3837,10 @@ class VPlan { | |||
| return cast<VPBasicBlock>(getVectorLoopRegion()->getSingleSuccessor()); | |||
| } | |||
|
|
|||
| /// Return the exit blocks of the VPlan, that is leaf nodes except the scalar | |||
| /// header. | |||
| auto getExitBlocks(); | |||
There was a problem hiding this comment.
If possible, it would be nice if this returned a concrete type or had a comment describing it. It's just a bit awkward for the reader having to look in a different header file to see what it returns.
There was a problem hiding this comment.
It is a bit unfortunate but I couldn't find a way to do that, as it needs accesses to the definition of VPBlockShallowTraversalWrapper to spell out the full type (other than moving everything in VPlanCFG back to this file).
There may be another way to spell out the type I am not aware of (it is a few nests of various iterator range adopters). For now I extended the comment to provide more information
| ExitIRI->addOperand(V); | ||
| for (VPIRBasicBlock *ExitVPBB : Plan.getExitBlocks()) { | ||
| BasicBlock *ExitBB = ExitVPBB->getIRBasicBlock(); | ||
| BasicBlock *ExitingBB = find_singleton<BasicBlock>( |
There was a problem hiding this comment.
Not sure how this works when all exiting blocks jump to the same exit block? LoopVectorizationLegality has a list of exiting and exit blocks that could perhaps be useful here? Otherwise you could compile a list of all exiting blocks for that exit block and walk over them.
This algorithm is definitely more powerful than what I originally planned in #88385, which isn't a bad thing. However, I guess it is more difficult to implement for the general case. If it's easier you could just limit this to the single early exit case that LoopVectorizationLegality current supports? That class already has the single early exiting block and early exit block.
There was a problem hiding this comment.
To handle multiple exiting blocks, this can be replaced by iterating over all exiting blocks when processing each exit phi I think, as in https://github.com/llvm/llvm-project/pull/112138/files#diff-da321d454a7246f8ae276bf1db2782bf26b5210b8133cb59e4d7fd45d0905decR8857
There was a problem hiding this comment.
Oh I see so this patch is only generalising for multiple exit blocks on the assumption that there is only a single predecessor per exit block. It looks like in PR #112138 you've generalised this further by removing that restriction. I guess this is ok and reduces the diff for PR #112138. In that case is it worth adding a temporary assert until PR #112138 lands?
assert(ExitingBB && "Expected exit block to have a single predecessor");
There was a problem hiding this comment.
The current version of the patch has an assert in addUsersInExitBlocks to make sure we only updated the exit phis, if the incoming value is from the middle block. Do you think that's sufficient or should I add another one here as suggested?
I'll update #112138 once this lands, and the assert in addUsersInExitBlocks would be turned into a bail-out as it means we encountered a exit value we cannot handle yet, and only will be able to with #88385,?
There was a problem hiding this comment.
Ah yes I see, that makes sense. In that case you're right what you have now is fine and happy for you to merge this patch!
| ExitIRI->addOperand(V); | ||
| for (VPIRBasicBlock *ExitVPBB : Plan.getExitBlocks()) { | ||
| BasicBlock *ExitBB = ExitVPBB->getIRBasicBlock(); | ||
| BasicBlock *ExitingBB = find_singleton<BasicBlock>( |
There was a problem hiding this comment.
To handle multiple exiting blocks, this can be replaced by iterating over all exiting blocks when processing each exit phi I think, as in https://github.com/llvm/llvm-project/pull/112138/files#diff-da321d454a7246f8ae276bf1db2782bf26b5210b8133cb59e4d7fd45d0905decR8857
| @@ -3837,6 +3837,10 @@ class VPlan { | |||
| return cast<VPBasicBlock>(getVectorLoopRegion()->getSingleSuccessor()); | |||
| } | |||
|
|
|||
| /// Return the exit blocks of the VPlan, that is leaf nodes except the scalar | |||
| /// header. | |||
| auto getExitBlocks(); | |||
There was a problem hiding this comment.
It is a bit unfortunate but I couldn't find a way to do that, as it needs accesses to the definition of VPBlockShallowTraversalWrapper to spell out the full type (other than moving everything in VPlanCFG back to this file).
There may be another way to spell out the type I am not aware of (it is a few nests of various iterator range adopters). For now I extended the comment to provide more information
| VPBlockUtils::blocksOnly<VPIRBasicBlock>( | ||
| vp_depth_first_shallow(getVectorLoopRegion()->getSingleSuccessor())), | ||
| [ScalarHeader](VPIRBasicBlock *VPIRBB) { | ||
| return VPIRBB != ScalarHeader && VPIRBB->getNumSuccessors() == 0; |
There was a problem hiding this comment.
It should always return the exit blocks currently in the VPlan, so if some transformation replaces/removes some of the exit blocks, the result would be different. But at the moment, the exit blocks should be fixed, we know them at time of construction and can wrap them in VPIRBasicBlocks and those shouldn't change (and if they change the transform would be responsible to make sure the exit phis are updated accordingly).
Is there anything particular in VPlan::execute you are thinking of? (We replace VPBasicBlocks with VPIRBasicBlocks in some cases if they are only created during skeleton creation, but that shouldn't be the case for the exit blocks)
Generalize collectUsersInExitBlock to collecting exit users in multiple exit blocks. Exit blocks are leaf nodes in the VPlan (without successors) except the scalar header.
Split off in preparation for #112138