-
Notifications
You must be signed in to change notification settings - Fork 18.1k
[VPlan] Split out EVL exit cond transform from canonicalizeEVLLoops. NFC #178181
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
Changes from 1 commit
753bdde
0a49ca0
f8952aa
c0e3ae7
0edc12b
26e6cb2
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 |
|---|---|---|
|
|
@@ -3247,9 +3247,9 @@ void VPlanTransforms::addExplicitVectorLength( | |
| Plan.setUF(1); | ||
| } | ||
|
|
||
| void VPlanTransforms::canonicalizeEVLLoops(VPlan &Plan) { | ||
| // Find EVL loop entries by locating VPEVLBasedIVPHIRecipe. | ||
| // There should be only one EVL PHI in the entire plan. | ||
| /// Find EVL loop entries by locating VPEVLBasedIVPHIRecipe. | ||
| /// There should be only one EVL PHI in the entire plan. | ||
|
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. Just to note that now we need to traverse the full VPlan twice if there's no EVL recipe. would be good if there would a good way to exit early one we can't find a EVL recipe in the header if possible. There's I think later we assert that the first phi in EVL's parent is a canonical IV, via below. Maybe we could try to check this early, and bail out once we found a block with a canonical IV-like but not a VPEVLBasedIVPHIRecipe recipes. Not sure if we may be able to check if the increment of the phi is used by the expected branch-on-cond.
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. Now that you mention it, we can just do the branch condition transform before the loop regions are dissolved and use getCanonicalIV, the EVL IV will immediately follow it if it exists 0a49ca0
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. moving makes sense, thanks |
||
| static VPEVLBasedIVPHIRecipe *findEVLPhi(VPlan &Plan) { | ||
| VPEVLBasedIVPHIRecipe *EVLPhi = nullptr; | ||
|
|
||
| for (VPBasicBlock *VPBB : VPBlockUtils::blocksOnly<VPBasicBlock>( | ||
|
|
@@ -3260,28 +3260,17 @@ void VPlanTransforms::canonicalizeEVLLoops(VPlan &Plan) { | |
| EVLPhi = PhiR; | ||
| } | ||
|
|
||
| return EVLPhi; | ||
| } | ||
|
|
||
| void VPlanTransforms::canonicalizeEVLLoops(VPlan &Plan) { | ||
| VPEVLBasedIVPHIRecipe *EVLPhi = findEVLPhi(Plan); | ||
| // Early return if no EVL PHI is found. | ||
| if (!EVLPhi) | ||
| return; | ||
|
|
||
| VPBasicBlock *HeaderVPBB = EVLPhi->getParent(); | ||
| VPValue *EVLIncrement = EVLPhi->getBackedgeValue(); | ||
| VPValue *AVL; | ||
| [[maybe_unused]] bool FoundAVL = | ||
| match(EVLIncrement, | ||
| m_c_Add(m_ZExtOrSelf(m_EVL(m_VPValue(AVL))), m_Specific(EVLPhi))); | ||
| assert(FoundAVL && "Didn't find AVL?"); | ||
|
|
||
| // The AVL may be capped to a safe distance. | ||
| VPValue *SafeAVL; | ||
| if (match(AVL, m_Select(m_VPValue(), m_VPValue(SafeAVL), m_VPValue()))) | ||
| AVL = SafeAVL; | ||
|
|
||
| VPValue *AVLNext; | ||
| [[maybe_unused]] bool FoundAVLNext = | ||
| match(AVL, m_VPInstruction<Instruction::PHI>( | ||
| m_Specific(Plan.getTripCount()), m_VPValue(AVLNext))); | ||
| assert(FoundAVLNext && "Didn't find AVL backedge?"); | ||
|
|
||
| // Convert EVLPhi to concrete recipe. | ||
| auto *ScalarR = | ||
|
|
@@ -3302,21 +3291,45 @@ void VPlanTransforms::canonicalizeEVLLoops(VPlan &Plan) { | |
| VPRecipeBase *CanonicalIVIncrement = Backedge->getDefiningRecipe(); | ||
| CanonicalIVIncrement->eraseFromParent(); | ||
| CanonicalIV->eraseFromParent(); | ||
| } | ||
|
|
||
| // Replace the use of VectorTripCount in the latch-exiting block. | ||
| // Before: (branch-on-cond (icmp eq EVLIVInc, VectorTripCount)) | ||
| // After: (branch-on-cond icmp eq AVLNext, 0) | ||
| void VPlanTransforms::convertEVLExitCond(VPlan &Plan) { | ||
| VPEVLBasedIVPHIRecipe *EVLPhi = findEVLPhi(Plan); | ||
| if (!EVLPhi) | ||
| return; | ||
|
|
||
| // Bail if not an EVL tail folded loop. | ||
| VPValue *AVL; | ||
| if (!match(EVLPhi->getBackedgeValue(), | ||
| m_c_Add(m_ZExtOrSelf(m_EVL(m_VPValue(AVL))), m_Specific(EVLPhi)))) | ||
| return; | ||
|
|
||
| // The AVL may be capped to a safe distance. | ||
| VPValue *SafeAVL; | ||
| if (match(AVL, m_Select(m_VPValue(), m_VPValue(SafeAVL), m_VPValue()))) | ||
|
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. not related to the patch itself, but it might be good to think if we can match something more precise here, in case some transform gets applied to the original select
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. Yeah, I think we can make this more explicit. Will try to do in a follow up.
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. Great thanks! |
||
| AVL = SafeAVL; | ||
|
|
||
| VPValue *AVLNext; | ||
| [[maybe_unused]] bool FoundAVLNext = | ||
| match(AVL, m_VPInstruction<Instruction::PHI>( | ||
| m_Specific(Plan.getTripCount()), m_VPValue(AVLNext))); | ||
| assert(FoundAVLNext && "Didn't find AVL backedge?"); | ||
|
|
||
| VPBasicBlock *HeaderVPBB = EVLPhi->getParent(); | ||
| VPBasicBlock *LatchExiting = | ||
| HeaderVPBB->getPredecessors()[1]->getEntryBasicBlock(); | ||
| auto *LatchExitingBr = cast<VPInstruction>(LatchExiting->getTerminator()); | ||
| if (match(LatchExitingBr, m_BranchOnCond(m_True()))) | ||
| return; | ||
|
|
||
| assert(match(LatchExitingBr, m_BranchOnCond(m_SpecificCmp( | ||
| CmpInst::ICMP_EQ, m_VPValue(EVLIncrement), | ||
|
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. This also fixes this m_VPValue, it should be a m_Specific |
||
| m_Specific(&Plan.getVectorTripCount())))) && | ||
| "Expected BranchOnCond with ICmp comparing EVL increment with vector " | ||
| "trip count"); | ||
| [[maybe_unused]] auto *CanIV = cast<VPPhi>(&*EVLPhi->getParent()->begin()); | ||
| assert( | ||
| match(LatchExitingBr, | ||
| m_BranchOnCond(m_SpecificCmp( | ||
| CmpInst::ICMP_EQ, m_Specific(CanIV->getIncomingValue(1)), | ||
| m_Specific(&Plan.getVectorTripCount())))) && | ||
| "Expected BranchOnCond with ICmp comparing CanIV increment with vector " | ||
| "trip count"); | ||
|
|
||
| Type *AVLTy = VPTypeAnalysis(Plan).inferScalarType(AVLNext); | ||
| VPBuilder Builder(LatchExitingBr); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -315,12 +315,14 @@ struct VPlanTransforms { | |
| /// variable vector lengths instead of fixed lengths. This transformation: | ||
| /// * Makes EVL-Phi concrete. | ||
| // * Removes CanonicalIV and increment. | ||
| /// * Replaces the exit condition from | ||
| /// (branch-on-count CanonicalIVInc, VectorTripCount) | ||
| /// to | ||
| /// (branch-on-cond eq AVLNext, 0) | ||
| static void canonicalizeEVLLoops(VPlan &Plan); | ||
|
|
||
| /// Replaces the exit condition from | ||
| /// (branch-on-count CanonicalIVInc, VectorTripCount) | ||
| /// to | ||
| /// (branch-on-cond eq AVLNext, 0) | ||
| static void convertEVLExitCond(VPlan &Plan); | ||
|
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. nit: there's no clear order to definitions, but
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. Fixed, thanks |
||
|
|
||
| /// Lower abstract recipes to concrete ones, that can be codegen'd. | ||
| static void convertToConcreteRecipes(VPlan &Plan); | ||
|
|
||
|
|
||
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.
nit: transform/convert may be more in line with owrding elswhere
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.
Fixed, thanks