[VPlan] Use VPlan predecessors in VPWidenPHIRecipe (NFC). #126388
+37
−39
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Update VPWidenPHIRecipe to use the predecessors in VPlan to determine the incoming blocks instead of tracking them separately. This brings VPWidenPHIrecipe in line with the other phi recipes.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-vectorizers Author: Florian Hahn (fhahn) ChangesUpdate VPWidenPHIRecipe to use the predecessors in VPlan to determine the incoming blocks instead of tracking them separately. This brings VPWidenPHIrecipe in line with the other phi recipes. Full diff: https://github.com/llvm/llvm-project/pull/126388.diff 6 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 3816e1b61576a3d..f2f118f5d2980c1 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -1958,13 +1958,12 @@ class VPScalarPHIRecipe : public VPHeaderPHIRecipe {
#endif
};
-/// A recipe for handling phis that are widened in the vector loop.
-/// In the VPlan native path, all incoming VPValues & VPBasicBlock pairs are
-/// managed in the recipe directly.
+/// A recipe for widened phis. Incoming values are operands of the recipe and
+/// their operand index corresponds to the incoming predeocessor block. If the
+/// recipe is placed in an entry block to a (non-replicate) region, it must have
+/// exactly 2 incoming values, from from the predecessors of the region and one
+/// from the exiting block of the region.
class VPWidenPHIRecipe : public VPSingleDefRecipe {
- /// List of incoming blocks. Only used in the VPlan native path.
- SmallVector<VPBasicBlock *, 2> IncomingBlocks;
-
public:
/// Create a new VPWidenPHIRecipe for \p Phi with start value \p Start and
/// debug location \p DL.
@@ -1991,19 +1990,8 @@ class VPWidenPHIRecipe : public VPSingleDefRecipe {
VPSlotTracker &SlotTracker) const override;
#endif
- /// Adds a pair (\p IncomingV, \p IncomingBlock) to the phi.
- void addIncoming(VPValue *IncomingV, VPBasicBlock *IncomingBlock) {
- addOperand(IncomingV);
- IncomingBlocks.push_back(IncomingBlock);
- }
-
/// Returns the \p I th incoming VPBasicBlock.
- VPBasicBlock *getIncomingBlock(unsigned I) { return IncomingBlocks[I]; }
-
- /// Set the \p I th incoming VPBasicBlock to \p IncomingBlock.
- void setIncomingBlock(unsigned I, VPBasicBlock *IncomingBlock) {
- IncomingBlocks[I] = IncomingBlock;
- }
+ VPBasicBlock *getIncomingBlock(unsigned I);
/// Returns the \p I th incoming VPValue.
VPValue *getIncomingValue(unsigned I) { return getOperand(I); }
diff --git a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
index 5a2e5d7cfee48d0..c94ea35e8112e25 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp
@@ -136,19 +136,22 @@ void PlainCFGBuilder::fixPhiNodes() {
// predecessor is the first operand of the recipe.
assert(Phi->getNumOperands() == 2);
BasicBlock *LoopPred = L->getLoopPredecessor();
- VPPhi->addIncoming(
- getOrCreateVPOperand(Phi->getIncomingValueForBlock(LoopPred)),
- BB2VPBB[LoopPred]);
+ VPPhi->addOperand(
+ getOrCreateVPOperand(Phi->getIncomingValueForBlock(LoopPred)));
BasicBlock *LoopLatch = L->getLoopLatch();
- VPPhi->addIncoming(
- getOrCreateVPOperand(Phi->getIncomingValueForBlock(LoopLatch)),
- BB2VPBB[LoopLatch]);
+ VPPhi->addOperand(
+ getOrCreateVPOperand(Phi->getIncomingValueForBlock(LoopLatch)));
continue;
}
- for (unsigned I = 0; I != Phi->getNumOperands(); ++I)
- VPPhi->addIncoming(getOrCreateVPOperand(Phi->getIncomingValue(I)),
- BB2VPBB[Phi->getIncomingBlock(I)]);
+ // Add operands for VPPhi in the order matching its predecessors in VPlan.
+ DenseMap<const VPBasicBlock *, VPValue *> IncomingValues;
+ for (unsigned I = 0; I != Phi->getNumOperands(); ++I) {
+ IncomingValues[BB2VPBB[Phi->getIncomingBlock(I)]] =
+ getOrCreateVPOperand(Phi->getIncomingValue(I));
+ }
+ for (VPBlockBase *Pred : VPPhi->getParent()->getPredecessors())
+ VPPhi->addOperand(IncomingValues.lookup(Pred->getExitingBasicBlock()));
}
}
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index bc80c5ea0b1b2af..b86b6285fc50183 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -3577,6 +3577,21 @@ void VPReductionPHIRecipe::print(raw_ostream &O, const Twine &Indent,
}
#endif
+VPBasicBlock *VPWidenPHIRecipe::getIncomingBlock(unsigned I) {
+ VPBasicBlock *Parent = getParent();
+ VPBlockBase *Pred = nullptr;
+ if (Parent->getNumPredecessors() == 0) {
+ auto *R = Parent->getParent();
+ assert(R && R->getEntry() == Parent);
+ assert(I < 2);
+ Pred = I == 0 ? R->getSinglePredecessor() : R;
+ } else {
+ Pred = Parent->getPredecessors()[I];
+ }
+
+ return Pred->getExitingBasicBlock();
+}
+
void VPWidenPHIRecipe::execute(VPTransformState &State) {
assert(EnableVPlanNativePath &&
"Non-native vplans are not expected to have VPWidenPHIRecipes.");
diff --git a/llvm/lib/Transforms/Vectorize/VPlanUtils.h b/llvm/lib/Transforms/Vectorize/VPlanUtils.h
index ac5e1978fcfbe00..6ddb88308955f1a 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanUtils.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanUtils.h
@@ -169,16 +169,8 @@ class VPBlockUtils {
static void reassociateBlocks(VPBlockBase *Old, VPBlockBase *New) {
for (auto *Pred : to_vector(Old->getPredecessors()))
Pred->replaceSuccessor(Old, New);
- for (auto *Succ : to_vector(Old->getSuccessors())) {
+ for (auto *Succ : to_vector(Old->getSuccessors()))
Succ->replacePredecessor(Old, New);
-
- // Replace any references to Old in widened phi incoming blocks.
- for (auto &R : Succ->getEntryBasicBlock()->phis())
- if (auto *WidenPhiR = dyn_cast<VPWidenPHIRecipe>(&R))
- for (unsigned I = 0; I < WidenPhiR->getNumOperands(); I++)
- if (WidenPhiR->getIncomingBlock(I) == Old)
- WidenPhiR->setIncomingBlock(I, cast<VPBasicBlock>(New));
- }
New->setPredecessors(Old->getPredecessors());
New->setSuccessors(Old->getSuccessors());
Old->clearPredecessors();
diff --git a/llvm/test/Transforms/LoopVectorize/outer-loop-wide-phis.ll b/llvm/test/Transforms/LoopVectorize/outer-loop-wide-phis.ll
index 3f81c0f5c822a2e..c5d2f6acf85b380 100644
--- a/llvm/test/Transforms/LoopVectorize/outer-loop-wide-phis.ll
+++ b/llvm/test/Transforms/LoopVectorize/outer-loop-wide-phis.ll
@@ -134,7 +134,7 @@ define void @wide_phi_2_predecessors_phi_ops_swapped(ptr noalias %A, ptr noalias
; CHECK-NEXT: [[WIDE_MASKED_GATHER:%.*]] = call <4 x i64> @llvm.masked.gather.v4i64.v4p0(<4 x ptr> [[TMP1]], i32 8, <4 x i1> splat (i1 true), <4 x i64> poison)
; CHECK-NEXT: br label %[[INNER_LATCH4]]
; CHECK: [[INNER_LATCH4]]:
-; CHECK-NEXT: [[VEC_PHI5:%.*]] = phi <4 x i64> [ zeroinitializer, %[[INNER_HEADER1]] ], [ [[WIDE_MASKED_GATHER]], %[[THEN3]] ]
+; CHECK-NEXT: [[VEC_PHI5:%.*]] = phi <4 x i64> [ [[WIDE_MASKED_GATHER]], %[[THEN3]] ], [ zeroinitializer, %[[INNER_HEADER1]] ]
; CHECK-NEXT: [[TMP2:%.*]] = add nsw <4 x i64> [[VEC_PHI5]], [[VEC_IND]]
; CHECK-NEXT: [[TMP3]] = add nsw <4 x i64> [[TMP2]], [[VEC_PHI2]]
; CHECK-NEXT: [[TMP4]] = add nuw nsw <4 x i64> [[VEC_PHI]], splat (i64 1)
diff --git a/llvm/unittests/Transforms/Vectorize/VPlanTest.cpp b/llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
index 2f37c08bd9f117d..b17725382e33c56 100644
--- a/llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
+++ b/llvm/unittests/Transforms/Vectorize/VPlanTest.cpp
@@ -672,7 +672,7 @@ TEST_F(VPBasicBlockTest, reassociateBlocks) {
auto *WidenPhi = new VPWidenPHIRecipe(nullptr);
IntegerType *Int32 = IntegerType::get(C, 32);
VPValue *Val = Plan.getOrAddLiveIn(ConstantInt::get(Int32, 1));
- WidenPhi->addIncoming(Val, VPBB1);
+ WidenPhi->addOperand(Val);
VPBB2->appendRecipe(WidenPhi);
VPBasicBlock *VPBBNew = Plan.createVPBasicBlock("VPBBNew");
@@ -693,7 +693,7 @@ TEST_F(VPBasicBlockTest, reassociateBlocks) {
auto *WidenPhi = new VPWidenPHIRecipe(nullptr);
IntegerType *Int32 = IntegerType::get(C, 32);
VPValue *Val = Plan.getOrAddLiveIn(ConstantInt::get(Int32, 1));
- WidenPhi->addIncoming(Val, VPBB1);
+ WidenPhi->addOperand(Val);
VPBB2->appendRecipe(WidenPhi);
VPBasicBlock *VPBBNew = Plan.createVPBasicBlock("VPBBNew");
|
|
#125481 made me think if tracking the incoming blocks separately is actually needed and it appears we can use the same approach as all other phi recipes I think |
lukel97
approved these changes
Feb 9, 2025
ayalz
reviewed
Feb 9, 2025
Comment on lines
+1964
to
+1965
| /// exactly 2 incoming values, from from the predecessors of the region and one | ||
| /// from the exiting block of the region. |
There was a problem hiding this comment.
Suggested change
| /// exactly 2 incoming values, from from the predecessors of the region and one | |
| /// from the exiting block of the region. | |
| /// exactly 2 incoming values, the first from the predecessors of the region and the second | |
| /// from the exiting block of the region. |
| VPBasicBlock *Parent = getParent(); | ||
| VPBlockBase *Pred = nullptr; | ||
| if (Parent->getNumPredecessors() == 0) { | ||
| auto *R = Parent->getParent(); |
There was a problem hiding this comment.
nit:
Suggested change
| auto *R = Parent->getParent(); | |
| auto *Region = Parent->getParent(); |
R is often used for Recipe.
Comment on lines
+3585
to
+3586
| assert(R && R->getEntry() == Parent); | ||
| assert(I < 2); |
There was a problem hiding this comment.
Error messages.
VPlans holding VPWidenPHIRecipes have only loop regions, which have a single predecessor - the preheader, even when nested?
Worth asserting R is not a replicating region.
| auto *R = Parent->getParent(); | ||
| assert(R && R->getEntry() == Parent); | ||
| assert(I < 2); | ||
| Pred = I == 0 ? R->getSinglePredecessor() : R; |
There was a problem hiding this comment.
If I == 1 then the enclosing region is returned, whose exiting block feeds the VPWidenPHIRecipe across the backedge? Worth a comment.
| @@ -136,19 +136,22 @@ void PlainCFGBuilder::fixPhiNodes() { | |||
| // predecessor is the first operand of the recipe. | |||
| assert(Phi->getNumOperands() == 2); | |||
There was a problem hiding this comment.
(independent) Error message.
| getOrCreateVPOperand(Phi->getIncomingValueForBlock(LoopPred)), | ||
| BB2VPBB[LoopPred]); | ||
| VPPhi->addOperand( | ||
| getOrCreateVPOperand(Phi->getIncomingValueForBlock(LoopPred))); |
There was a problem hiding this comment.
(independent) This first operand can be set when creating the VPWidenPHIRecipe, only the second operand requires backpatching.
| for (unsigned I = 0; I != Phi->getNumOperands(); ++I) | ||
| VPPhi->addIncoming(getOrCreateVPOperand(Phi->getIncomingValue(I)), | ||
| BB2VPBB[Phi->getIncomingBlock(I)]); | ||
| // Add operands for VPPhi in the order matching its predecessors in VPlan. |
There was a problem hiding this comment.
(independent) The operands of non-header VPWidenPHIRecipes can be set when creating it, rather than here.
| VPPhi->addIncoming(getOrCreateVPOperand(Phi->getIncomingValue(I)), | ||
| BB2VPBB[Phi->getIncomingBlock(I)]); | ||
| // Add operands for VPPhi in the order matching its predecessors in VPlan. | ||
| DenseMap<const VPBasicBlock *, VPValue *> IncomingValues; |
There was a problem hiding this comment.
Suggested change
| DenseMap<const VPBasicBlock *, VPValue *> IncomingValues; | |
| DenseMap<const VPBasicBlock *, VPValue *> VPPredToIncomingValue; |
| getOrCreateVPOperand(Phi->getIncomingValue(I)); | ||
| } | ||
| for (VPBlockBase *Pred : VPPhi->getParent()->getPredecessors()) | ||
| VPPhi->addOperand(IncomingValues.lookup(Pred->getExitingBasicBlock())); |
There was a problem hiding this comment.
Note: if/when the reverse mapping VPBB2BB is available, this could be done by
VPPhi->addOperand(Phi->getIncomingValueForBlock(VPBB2BB[Pred->getExitingBasicBlock()]));
| void setIncomingBlock(unsigned I, VPBasicBlock *IncomingBlock) { | ||
| IncomingBlocks[I] = IncomingBlock; | ||
| } | ||
| VPBasicBlock *getIncomingBlock(unsigned I); |
There was a problem hiding this comment.
Follow-up: make VPWidenPHIRecipe a VPInstruction by inlining both getIncomingValue() and getIncomingBlock() (in VPlan the latter fits a VPBB rather than a phi recipe) into their only user - fixNonInductionPHIs().
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Update VPWidenPHIRecipe to use the predecessors in VPlan to determine the incoming blocks instead of tracking them separately. This brings VPWidenPHIrecipe in line with the other phi recipes.