-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[VPlan] Move addExplicitVectorLength to tryToBuildVPlanWithVPRecipes #166164
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 all commits
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 | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -605,9 +605,12 @@ createScalarIVSteps(VPlan &Plan, InductionDescriptor::InductionKind Kind, | |||||||||||||||||||||||||||||||||||||||||||||||||||
| VPBuilder &Builder) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPRegionBlock *LoopRegion = Plan.getVectorLoopRegion(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPBasicBlock *HeaderVPBB = LoopRegion->getEntryBasicBlock(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPCanonicalIVPHIRecipe *CanonicalIV = LoopRegion->getCanonicalIV(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPSingleDefRecipe *BaseIV = Builder.createDerivedIV( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Kind, FPBinOp, StartV, CanonicalIV, Step, "offset.idx"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPHeaderPHIRecipe *IV = LoopRegion->getCanonicalIV(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (auto *EVLIV = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| dyn_cast<VPEVLBasedIVPHIRecipe>(std::next(IV->getIterator()))) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| IV = EVLIV; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPSingleDefRecipe *BaseIV = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Builder.createDerivedIV(Kind, FPBinOp, StartV, IV, Step, "offset.idx"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Truncate base induction if needed. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPTypeAnalysis TypeInfo(Plan); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2621,8 +2624,42 @@ static VPRecipeBase *optimizeMaskToEVL(VPValue *HeaderMask, | |||||||||||||||||||||||||||||||||||||||||||||||||||
| return nullptr; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Replace recipes with their EVL variants. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| static void transformRecipestoEVLRecipes(VPlan &Plan, VPValue &EVL) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| void VPlanTransforms::optimizeMasksToEVL(VPlan &Plan) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Find the EVL-based header mask if it exists: icmp ult step-vector, EVL | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPInstruction *HeaderMask = nullptr; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (VPRecipeBase &R : *Plan.getVectorLoopRegion()->getEntryBasicBlock()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (match(&R, m_ICmp(m_VPInstruction<VPInstruction::StepVector>(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| m_EVL(m_VPValue())))) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| HeaderMask = cast<VPInstruction>(&R); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!HeaderMask) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPValue *EVL = HeaderMask->getOperand(1); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+2628
to
+2640
Contributor
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. Do you have plan to create helper function like
Suggested change
?
Contributor
Author
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. I wasn't planning on creating a helper function. I think the change you suggested matches the AVL though, not the EVL. I think we need something like
Contributor
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. BTW, do we need to update vputils::isHeaderMask?
Contributor
Author
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. I don't think so, isHeaderMask is only used by findHeaderMask, which in turn is only used by transformRecipestoEVLRecipes and addActiveLaneMask |
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPTypeAnalysis TypeInfo(Plan); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (VPUser *U : collectUsersRecursively(HeaderMask)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPRecipeBase *R = cast<VPRecipeBase>(U); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (auto *NewR = optimizeMaskToEVL(HeaderMask, *R, TypeInfo, *EVL)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| NewR->insertBefore(R); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (auto [Old, New] : | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| zip_equal(R->definedValues(), NewR->definedValues())) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Old->replaceAllUsesWith(New); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Erase dead stores, the rest will be removed by removeDeadRecipes. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (R->getNumDefinedValues() == 0) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| R->eraseFromParent(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| removeDeadRecipes(Plan); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// After replacing the IV with a EVL-based IV, fixup recipes that use VF to use | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// the EVL instead to avoid incorrect updates on the penultimate iteration. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| static void fixupVFUsersForEVL(VPlan &Plan, VPValue &EVL) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPTypeAnalysis TypeInfo(Plan); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPRegionBlock *LoopRegion = Plan.getVectorLoopRegion(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPBasicBlock *Header = LoopRegion->getEntryBasicBlock(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2650,10 +2687,6 @@ static void transformRecipestoEVLRecipes(VPlan &Plan, VPValue &EVL) { | |||||||||||||||||||||||||||||||||||||||||||||||||||
| return isa<VPWidenPointerInductionRecipe>(U); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Defer erasing recipes till the end so that we don't invalidate the | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // VPTypeAnalysis cache. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| SmallVector<VPRecipeBase *> ToErase; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Create a scalar phi to track the previous EVL if fixed-order recurrence is | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // contained. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| bool ContainsFORs = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2687,7 +2720,6 @@ static void transformRecipestoEVLRecipes(VPlan &Plan, VPValue &EVL) { | |||||||||||||||||||||||||||||||||||||||||||||||||||
| TypeInfo.inferScalarType(R.getVPSingleValue()), R.getDebugLoc()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPSplice->insertBefore(&R); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| R.getVPSingleValue()->replaceAllUsesWith(VPSplice); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ToErase.push_back(&R); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2708,43 +2740,6 @@ static void transformRecipestoEVLRecipes(VPlan &Plan, VPValue &EVL) { | |||||||||||||||||||||||||||||||||||||||||||||||||||
| CmpInst::ICMP_ULT, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| Builder.createNaryOp(VPInstruction::StepVector, {}, EVLType), &EVL); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| HeaderMask->replaceAllUsesWith(EVLMask); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ToErase.push_back(HeaderMask->getDefiningRecipe()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Try to optimize header mask recipes away to their EVL variants. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // TODO: Split optimizeMaskToEVL out and move into | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // VPlanTransforms::optimize. transformRecipestoEVLRecipes should be run in | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // tryToBuildVPlanWithVPRecipes beforehand. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (VPUser *U : collectUsersRecursively(EVLMask)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| auto *CurRecipe = cast<VPRecipeBase>(U); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPRecipeBase *EVLRecipe = | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| optimizeMaskToEVL(EVLMask, *CurRecipe, TypeInfo, EVL); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!EVLRecipe) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| unsigned NumDefVal = EVLRecipe->getNumDefinedValues(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert(NumDefVal == CurRecipe->getNumDefinedValues() && | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| "New recipe must define the same number of values as the " | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| "original."); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| EVLRecipe->insertBefore(CurRecipe); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (isa<VPSingleDefRecipe, VPWidenLoadEVLRecipe, VPInterleaveEVLRecipe>( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| EVLRecipe)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (unsigned I = 0; I < NumDefVal; ++I) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| VPValue *CurVPV = CurRecipe->getVPValue(I); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| CurVPV->replaceAllUsesWith(EVLRecipe->getVPValue(I)); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ToErase.push_back(CurRecipe); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Remove dead EVL mask. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (EVLMask->getNumUsers() == 0) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ToErase.push_back(EVLMask->getDefiningRecipe()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (VPRecipeBase *R : reverse(ToErase)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| SmallVector<VPValue *> PossiblyDead(R->operands()); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| R->eraseFromParent(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| for (VPValue *Op : PossiblyDead) | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| recursivelyDeleteDeadRecipes(Op); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Add a VPEVLBasedIVPHIRecipe and related recipes to \p Plan and | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -2842,7 +2837,7 @@ void VPlanTransforms::addExplicitVectorLength( | |||||||||||||||||||||||||||||||||||||||||||||||||||
| DebugLoc::getCompilerGenerated(), "avl.next"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| AVLPhi->addOperand(NextAVL); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| transformRecipestoEVLRecipes(Plan, *VPEVL); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| fixupVFUsersForEVL(Plan, *VPEVL); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Replace all uses of VPCanonicalIVPHIRecipe by | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| // VPEVLBasedIVPHIRecipe except for the canonical IV increment. | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
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.
Do you think we can avoid this by deferring canonical IV replacement until canonicalizeEVLLoops?
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.
I don't think thats a good idea because it means we will have an incorrect VPlan throughout the optimisation pipeline.
Part of the motivation for this PR is to have everything use the EVL based IV as soon as it's added so we don't accidentally have recipes using the canonical IV and producing incorrect results on the penultimate iteration.
We could probably add a method to VPRegionBlock that abstracts over the EVL or canonical IV like
getEffectiveIV, but that probably requires more discussion so I'd like to leave that to another PR if possibleThere 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.
Why would it be incorrect?
If the movement means every optimization must be careful whether to use the canonical IV or EVL based IV, and adding new users to the canonical IV could cause incorrect transformations, then I am not sure if that is the best direction forward?
Uh oh!
There was an error while loading. Please reload this page.
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.
Once we change the header mask to an EVL based mask (a.k.a variable stepping), if a widened recipe still uses the canonical IV it will operate on the wrong lanes in the penultimate iteration.
So the conversion of the header mask to the EVL based mask needs to be done in tandem with replacing all uses of the canonical IV with the EVL based IV.
This is already something we need to be careful about today, even without this patch. E.g.
narrowInterleaveGroupsuses the canonical IV and runs afteraddExplicitVectorLength. It just so happens to bail when it sees any non-canonical IV phis at the moment, but in the future we presumably will need to handle EVL based IVs etc.It crossed my mind that maybe we should just call
addExplicitVectorLengthas late as possible, but I can see two potential issues:In my opinion I think it's simplest if we have the EVL based loop early on, instead of having a mix of some transforms being EVL-aware and some unaware.
We should probably also audit users of
getCanonicalIVand make sure they're using some API that returns either the canonical IV or EVL based IV.Hope that explanation makes sense, open to other thoughts and suggestions.