Skip to content

[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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8301,7 +8301,7 @@ VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe(VPSingleDefRecipe *R,
VPRecipeBase *Recipe;
Instruction *Instr = R->getUnderlyingInstr();
SmallVector<VPValue *, 4> Operands(R->operands());
if (auto *PhiR = dyn_cast<VPWidenPHIRecipe>(R)) {
if (auto *PhiR = dyn_cast<VPPhi>(R)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (auto *PhiR = dyn_cast<VPPhi>(R)) {
assert(!isa<VPWidenPHIRecipe>(R) && "Only scalar recipes expected);
if (auto *PhiR = dyn_cast<VPPhi>(R)) {

?

Copy link
Contributor Author

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()

VPBasicBlock *Parent = PhiR->getParent();
[[maybe_unused]] VPRegionBlock *LoopRegionOf =
Parent->getEnclosingLoopRegion();
Expand Down Expand Up @@ -8339,6 +8339,7 @@ VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe(VPSingleDefRecipe *R,
PhiRecipe->addOperand(Operands[1]);
return PhiRecipe;
}
assert(!R->isPhi() && "only VPPhi nodes expected at this point");

if (isa<TruncInst>(Instr) && (Recipe = tryToOptimizeInductionTruncate(
cast<TruncInst>(Instr), Operands, Range)))
Expand Down
18 changes: 15 additions & 3 deletions llvm/lib/Transforms/Vectorize/VPlan.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also holds for above classof, for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Expand Down
16 changes: 7 additions & 9 deletions llvm/lib/Transforms/Vectorize/VPlanConstruction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
}
}
Expand Down Expand Up @@ -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");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent: should createScalarPhi() be called createVPPhi() or should VPPhi be called ScalarPhi?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

@ayalz ayalz Aug 5, 2025

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.
Expand Down
6 changes: 3 additions & 3 deletions llvm/lib/Transforms/Vectorize/VPlanPredicator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -227,10 +227,10 @@ void VPPredicator::createSwitchEdgeMasks(VPInstruction *SI) {
}

void VPPredicator::convertPhisToBlends(VPBasicBlock *VPBB) {
SmallVector<VPWidenPHIRecipe *> Phis;
SmallVector<VPPhi *> Phis;
for (VPRecipeBase &R : VPBB->phis())
Phis.push_back(cast<VPWidenPHIRecipe>(&R));
for (VPWidenPHIRecipe *PhiR : Phis) {
Phis.push_back(cast<VPPhi>(&R));
for (VPPhi *PhiR : Phis) {
// The non-header Phi is converted into a Blend recipe below,
// so we don't have to worry about the insertion order and we can just use
// the builder. At this point we generate the predication tree. There may
Expand Down
23 changes: 13 additions & 10 deletions llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent: Ingredient usually stands for an underlying IR Instruction rather than recipe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ define void @test(ptr %src, i64 %n) {
; CHECK-NEXT: [[TMP3:%.*]] = icmp eq <4 x i64> [[TMP2]], [[BROADCAST_SPLAT]]
; CHECK-NEXT: [[TMP4:%.*]] = extractelement <4 x i1> [[TMP3]], i32 0
; CHECK-NEXT: br i1 [[TMP4]], label [[LOOP_2_LATCH4]], label [[LOOP_32]]
; CHECK: loop.2.latch4:
; CHECK: loop.2.latch3:
; CHECK-NEXT: [[TMP5]] = add nuw nsw <4 x i64> [[VEC_PHI]], splat (i64 1)
; CHECK-NEXT: [[TMP6:%.*]] = icmp eq <4 x i64> [[TMP5]], [[BROADCAST_SPLAT]]
; CHECK-NEXT: [[TMP7:%.*]] = extractelement <4 x i1> [[TMP6]], i32 0
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ define void @foo(i64 %n) {
; CHECK-NEXT: Successor(s): outer.header
; CHECK-EMPTY:
; CHECK-NEXT: outer.header:
; CHECK-NEXT: WIDEN-PHI ir<%outer.iv> = phi [ ir<%outer.iv.next>, outer.latch ], [ ir<0>, ir-bb<entry> ]
; CHECK-NEXT: EMIT-SCALAR ir<%outer.iv> = phi [ ir<%outer.iv.next>, outer.latch ], [ ir<0>, ir-bb<entry> ]
; CHECK-NEXT: EMIT ir<%gep.1> = getelementptr ir<@arr2>, ir<0>, ir<%outer.iv>
; CHECK-NEXT: EMIT store ir<%outer.iv>, ir<%gep.1>
; CHECK-NEXT: EMIT ir<%add> = add ir<%outer.iv>, ir<%n>
; CHECK-NEXT: Successor(s): inner
; CHECK-EMPTY:
; CHECK-NEXT: inner:
; CHECK-NEXT: WIDEN-PHI ir<%inner.iv> = phi [ ir<%inner.iv.next>, inner ], [ ir<0>, outer.header ]
; CHECK-NEXT: EMIT-SCALAR ir<%inner.iv> = phi [ ir<%inner.iv.next>, inner ], [ ir<0>, outer.header ]
; CHECK-NEXT: EMIT ir<%gep.2> = getelementptr ir<@arr>, ir<0>, ir<%inner.iv>, ir<%outer.iv>
; CHECK-NEXT: EMIT store ir<%add>, ir<%gep.2>
; CHECK-NEXT: EMIT ir<%inner.iv.next> = add ir<%inner.iv>, ir<1>
Expand Down
6 changes: 3 additions & 3 deletions llvm/unittests/Transforms/Vectorize/VPlanHCFGTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ TEST_F(VPlanHCFGTest, testBuildHCFGInnerLoop) {
auto Iter = VecBB->begin();
auto *CanIV = dyn_cast<VPCanonicalIVPHIRecipe>(&*Iter++);
EXPECT_NE(nullptr, CanIV);
VPWidenPHIRecipe *Phi = dyn_cast<VPWidenPHIRecipe>(&*Iter++);
auto *Phi = dyn_cast<VPPhi>(&*Iter++);
EXPECT_NE(nullptr, Phi);

VPInstruction *Idx = dyn_cast<VPInstruction>(&*Iter++);
Expand Down Expand Up @@ -138,7 +138,7 @@ compound=true
N4 [label =
"vector.body:\l" +
" EMIT vp\<%2\> = CANONICAL-INDUCTION ir\<0\>, vp\<%index.next\>\l" +
" WIDEN-PHI ir\<%indvars.iv\> = phi [ ir\<0\>, vector.ph ], [ ir\<%indvars.iv.next\>, vector.body ]\l" +
" EMIT-SCALAR ir\<%indvars.iv\> = phi [ ir\<0\>, vector.ph ], [ ir\<%indvars.iv.next\>, vector.body ]\l" +
" EMIT ir\<%arr.idx\> = getelementptr ir\<%A\>, ir\<%indvars.iv\>\l" +
" EMIT ir\<%l1\> = load ir\<%arr.idx\>\l" +
" EMIT ir\<%res\> = add ir\<%l1\>, ir\<10\>\l" +
Expand Down Expand Up @@ -304,7 +304,7 @@ compound=true
N4 [label =
"vector.body:\l" +
" EMIT vp\<%2\> = CANONICAL-INDUCTION ir\<0\>, vp\<%index.next\>\l" +
" WIDEN-PHI ir\<%iv\> = phi [ ir\<0\>, vector.ph ], [ ir\<%iv.next\>, loop.latch ]\l" +
" EMIT-SCALAR ir\<%iv\> = phi [ ir\<0\>, vector.ph ], [ ir\<%iv.next\>, loop.latch ]\l" +
" EMIT ir\<%arr.idx\> = getelementptr ir\<%A\>, ir\<%iv\>\l" +
" EMIT ir\<%l1\> = load ir\<%arr.idx\>\l" +
" EMIT ir\<%c\> = icmp ir\<%l1\>, ir\<0\>\l" +
Expand Down