-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[VPlan] Move getCanonicalIV to VPRegionBlock (NFC). #163020
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 1 commit
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 |
---|---|---|
|
@@ -4066,6 +4066,16 @@ class LLVM_ABI_FOR_TEST VPRegionBlock : public VPBlockBase { | |
/// Remove the current region from its VPlan, connecting its predecessor to | ||
/// its entry, and its exiting block to its successor. | ||
void dissolveToCFGLoop(); | ||
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 dissolveToCFGLoop() also call getCanonicalIV(), possibly asserting that it resides in the-header/its-front/the-region? 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. Currently getCanonicalIV assumes that the region must have a canonical IV, which may not be the case at the moment (e.g. the inner loop region when vectorizing the outer loop does not have a canonical IV at the moment). |
||
|
||
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 VPWidenIntOrFpInductionRecipe::isCanonical() also call getCanonicalIV() instead of checking first recipe of header directly? 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, thanks |
||
/// Returns the canonical induction recipe of the region. | ||
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 VPlanVerifier::verify() (continue to) avoid calling getCanonicalIV() as it would assert rather than return false in case of failure? 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. |
||
VPCanonicalIVPHIRecipe *getCanonicalIV() { | ||
VPBasicBlock *EntryVPBB = getEntryBasicBlock(); | ||
if (EntryVPBB->empty()) { | ||
// VPlan native path. | ||
|
||
EntryVPBB = cast<VPBasicBlock>(EntryVPBB->getSingleSuccessor()); | ||
} | ||
return cast<VPCanonicalIVPHIRecipe>(&*EntryVPBB->begin()); | ||
} | ||
}; | ||
|
||
/// VPlan models a candidate for vectorization, encoding various decisions take | ||
|
@@ -4377,16 +4387,6 @@ class VPlan { | |
LLVM_DUMP_METHOD void dump() const; | ||
#endif | ||
|
||
/// Returns the canonical induction recipe of the vector loop. | ||
VPCanonicalIVPHIRecipe *getCanonicalIV() { | ||
VPBasicBlock *EntryVPBB = getVectorLoopRegion()->getEntryBasicBlock(); | ||
if (EntryVPBB->empty()) { | ||
// VPlan native path. | ||
EntryVPBB = cast<VPBasicBlock>(EntryVPBB->getSingleSuccessor()); | ||
} | ||
return cast<VPCanonicalIVPHIRecipe>(&*EntryVPBB->begin()); | ||
} | ||
|
||
VPValue *getSCEVExpansion(const SCEV *S) const { | ||
return SCEVToExpansion.lookup(S); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -661,9 +661,11 @@ void VPlanTransforms::attachCheckBlock(VPlan &Plan, Value *Cond, | |
} | ||
|
||
VPIRMetadata VPBranchWeights; | ||
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 VPBranchWeights needed? |
||
auto *Term = VPBuilder(CheckBlockVPBB) | ||
.createNaryOp(VPInstruction::BranchOnCond, {CondVPV}, | ||
Plan.getCanonicalIV()->getDebugLoc()); | ||
auto *Term = | ||
VPBuilder(CheckBlockVPBB) | ||
.createNaryOp( | ||
VPInstruction::BranchOnCond, {CondVPV}, | ||
Plan.getVectorLoopRegion()->getCanonicalIV()->getDebugLoc()); | ||
if (AddBranchWeights) { | ||
MDBuilder MDB(Plan.getContext()); | ||
MDNode *BranchWeights = | ||
|
@@ -925,8 +927,8 @@ bool VPlanTransforms::handleMaxMinNumReductions(VPlan &Plan) { | |
if (auto *DerivedIV = dyn_cast<VPDerivedIVRecipe>(VecV)) { | ||
if (DerivedIV->getNumUsers() == 1 && | ||
DerivedIV->getOperand(1) == &Plan.getVectorTripCount()) { | ||
auto *NewSel = Builder.createSelect(AnyNaN, Plan.getCanonicalIV(), | ||
&Plan.getVectorTripCount()); | ||
auto *NewSel = Builder.createSelect( | ||
AnyNaN, LoopRegion->getCanonicalIV(), &Plan.getVectorTripCount()); | ||
DerivedIV->moveAfter(&*Builder.getInsertPoint()); | ||
DerivedIV->setOperand(1, NewSel); | ||
continue; | ||
|
@@ -939,7 +941,8 @@ bool VPlanTransforms::handleMaxMinNumReductions(VPlan &Plan) { | |
"FMaxNum/FMinNum reduction.\n"); | ||
return false; | ||
} | ||
auto *NewSel = Builder.createSelect(AnyNaN, Plan.getCanonicalIV(), VecV); | ||
auto *NewSel = | ||
Builder.createSelect(AnyNaN, LoopRegion->getCanonicalIV(), VecV); | ||
ResumeR->setOperand(0, NewSel); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -168,7 +168,8 @@ void VPPredicator::createHeaderMask(VPBasicBlock *HeaderVPBB, bool FoldTail) { | |
// non-phi instructions. | ||
|
||
auto &Plan = *HeaderVPBB->getPlan(); | ||
auto *IV = new VPWidenCanonicalIVRecipe(Plan.getCanonicalIV()); | ||
auto *IV = new VPWidenCanonicalIVRecipe( | ||
Plan.getVectorLoopRegion()->getCanonicalIV()); | ||
|
||
Builder.setInsertPoint(HeaderVPBB, HeaderVPBB->getFirstNonPhi()); | ||
Builder.insert(IV); | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -501,7 +501,8 @@ static void removeRedundantInductionCasts(VPlan &Plan) { | |||||||||||
/// Try to replace VPWidenCanonicalIVRecipes with a widened canonical IV | ||||||||||||
/// recipe, if it exists. | ||||||||||||
Comment on lines
501
to
502
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:
Suggested change
only one (the first) VPWidenCanonicalIVRecipe is replaced. Try to replace them all? |
||||||||||||
static void removeRedundantCanonicalIVs(VPlan &Plan) { | ||||||||||||
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: shorten name or extend implementation to match it
Suggested change
|
||||||||||||
VPCanonicalIVPHIRecipe *CanonicalIV = Plan.getCanonicalIV(); | ||||||||||||
VPRegionBlock *LoopRegion = Plan.getVectorLoopRegion(); | ||||||||||||
VPCanonicalIVPHIRecipe *CanonicalIV = LoopRegion->getCanonicalIV(); | ||||||||||||
VPWidenCanonicalIVRecipe *WidenNewIV = nullptr; | ||||||||||||
for (VPUser *U : CanonicalIV->users()) { | ||||||||||||
WidenNewIV = dyn_cast<VPWidenCanonicalIVRecipe>(U); | ||||||||||||
|
@@ -512,7 +513,7 @@ static void removeRedundantCanonicalIVs(VPlan &Plan) { | |||||||||||
if (!WidenNewIV) | ||||||||||||
return; | ||||||||||||
|
||||||||||||
VPBasicBlock *HeaderVPBB = Plan.getVectorLoopRegion()->getEntryBasicBlock(); | ||||||||||||
VPBasicBlock *HeaderVPBB = LoopRegion->getEntryBasicBlock(); | ||||||||||||
for (VPRecipeBase &Phi : HeaderVPBB->phis()) { | ||||||||||||
auto *WidenOriginalIV = dyn_cast<VPWidenIntOrFpInductionRecipe>(&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: if "all users of WidenNewIV demand the first lane only" below - then why is it widened? Holds to recipes generating vectors in general when only their first lane is needed. |
||||||||||||
|
@@ -582,8 +583,9 @@ createScalarIVSteps(VPlan &Plan, InductionDescriptor::InductionKind Kind, | |||||||||||
FPMathOperator *FPBinOp, Instruction *TruncI, | ||||||||||||
VPValue *StartV, VPValue *Step, DebugLoc DL, | ||||||||||||
VPBuilder &Builder) { | ||||||||||||
VPBasicBlock *HeaderVPBB = Plan.getVectorLoopRegion()->getEntryBasicBlock(); | ||||||||||||
VPCanonicalIVPHIRecipe *CanonicalIV = Plan.getCanonicalIV(); | ||||||||||||
VPRegionBlock *LoopRegion = Plan.getVectorLoopRegion(); | ||||||||||||
VPBasicBlock *HeaderVPBB = LoopRegion->getEntryBasicBlock(); | ||||||||||||
VPCanonicalIVPHIRecipe *CanonicalIV = LoopRegion->getCanonicalIV(); | ||||||||||||
VPSingleDefRecipe *BaseIV = Builder.createDerivedIV( | ||||||||||||
Kind, FPBinOp, StartV, CanonicalIV, Step, "offset.idx"); | ||||||||||||
|
||||||||||||
|
@@ -800,8 +802,9 @@ static VPValue *optimizeEarlyExitInductionUser(VPlan &Plan, | |||||||||||
return nullptr; | ||||||||||||
|
||||||||||||
// Calculate the final index. | ||||||||||||
VPValue *EndValue = Plan.getCanonicalIV(); | ||||||||||||
auto CanonicalIVType = Plan.getCanonicalIV()->getScalarType(); | ||||||||||||
VPRegionBlock *LoopRegion = Plan.getVectorLoopRegion(); | ||||||||||||
VPValue *EndValue = LoopRegion->getCanonicalIV(); | ||||||||||||
auto CanonicalIVType = LoopRegion->getCanonicalIV()->getScalarType(); | ||||||||||||
|
VPValue *EndValue = LoopRegion->getCanonicalIV(); | |
auto CanonicalIVType = LoopRegion->getCanonicalIV()->getScalarType(); | |
auto *CanonicalIV = LoopRegion->getCanonicalIV(); | |
auto CanonicalIVType = CanonicalIV->getScalarType(); | |
VPValue *EndValue = CanonicalIV; |
or better define EndValue below using
VPValue *EndValue = B.createNaryOp(Instruction::Add, {CanonicalIV, FirstActiveLane}, DL);
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.
Done thanks
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -69,7 +69,8 @@ class UnrollState { | |||||
VPBasicBlock::iterator InsertPtForPhi); | ||||||
|
||||||
VPValue *getConstantVPV(unsigned Part) { | ||||||
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:
Suggested change
|
||||||
Type *CanIVIntTy = Plan.getCanonicalIV()->getScalarType(); | ||||||
Type *CanIVIntTy = | ||||||
Plan.getVectorLoopRegion()->getCanonicalIV()->getScalarType(); | ||||||
Comment on lines
+72
to
+73
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 have TypeInfo supply CanIVIntTy? |
||||||
return Plan.getOrAddLiveIn(ConstantInt::get(CanIVIntTy, Part)); | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -67,8 +67,10 @@ bool vputils::isHeaderMask(const VPValue *V, VPlan &Plan) { | |||||
|
||||||
if (match(V, m_ActiveLaneMask(m_VPValue(A), m_VPValue(B), m_One()))) | ||||||
return B == Plan.getTripCount() && | ||||||
(match(A, m_ScalarIVSteps(m_Specific(Plan.getCanonicalIV()), m_One(), | ||||||
m_Specific(&Plan.getVF()))) || | ||||||
(match(A, | ||||||
m_ScalarIVSteps( | ||||||
m_Specific(Plan.getVectorLoopRegion()->getCanonicalIV()), | ||||||
m_One(), m_Specific(&Plan.getVF()))) || | ||||||
IsWideCanonicalIV(A)); | ||||||
|
||||||
return match(V, m_ICmp(m_VPValue(A), m_VPValue(B))) && IsWideCanonicalIV(A) && | ||||||
|
@@ -102,7 +104,8 @@ bool vputils::isUniformAcrossVFsAndUFs(VPValue *V) { | |||||
return all_of(R->operands(), isUniformAcrossVFsAndUFs); | ||||||
} | ||||||
|
||||||
auto *CanonicalIV = R->getParent()->getPlan()->getCanonicalIV(); | ||||||
auto *CanonicalIV = | ||||||
R->getParent()->getPlan()->getVectorLoopRegion()->getCanonicalIV(); | ||||||
|
R->getParent()->getPlan()->getVectorLoopRegion()->getCanonicalIV(); | |
R->getParent()->getEnclosingLoopRegion()->getCanonicalIV(); |
?
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.
done thanks
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.
Note that LoopRegion is retrieved again below in line 8296.
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.
Unified, thanks