-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[LV] Vectorize selecting last IV of min/max element. #141431
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 all commits
11870a2
2688d03
26ebf5a
af2ba25
2871d6c
ae18690
4305caf
98fb1f7
dc59607
6134ef1
ad99496
5b65693
fabcf69
844c2c2
df7e6b8
5e209db
127da7d
603c47c
76e661a
d74939e
a057dd3
c351e55
fd90ad9
2fd21ec
da55075
5a442b2
a78311d
87325fd
918f079
6cc3953
3cedf8a
b1ff1a4
895baa8
a073c9b
6d8e164
0671371
450d6a0
cb25d0c
7d34974
7710b71
7e2d9c3
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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -216,6 +216,52 @@ static bool checkOrderedReduction(RecurKind Kind, Instruction *ExactFPMathInst, | |||||||
| return true; | ||||||||
| } | ||||||||
|
|
||||||||
| /// Returns true if \p Phi is a min/max reduction matching \p Kind where \p Phi | ||||||||
| /// is used outside the reduction chain. This is common for loops selecting the | ||||||||
| /// index of a minimum/maximum value (argmin/argmax). | ||||||||
| static bool isMinMaxReductionPhiWithUsersOutsideReductionChain( | ||||||||
| PHINode *Phi, RecurKind Kind, Loop *TheLoop, RecurrenceDescriptor &RedDes) { | ||||||||
| BasicBlock *Latch = TheLoop->getLoopLatch(); | ||||||||
| if (!Latch) | ||||||||
| return false; | ||||||||
|
Collaborator
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.
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. added, thansk |
||||||||
|
|
||||||||
| assert(Phi->getNumIncomingValues() == 2 && "phi must have 2 incoming values"); | ||||||||
| Value *Inc = Phi->getIncomingValueForBlock(Latch); | ||||||||
| if (Phi->hasOneUse() || !Inc->hasOneUse() || | ||||||||
| !RecurrenceDescriptor::isIntMinMaxRecurrenceKind(Kind)) | ||||||||
| return false; | ||||||||
|
|
||||||||
| Value *A, *B; | ||||||||
| bool IsMinMax = [&]() { | ||||||||
| switch (Kind) { | ||||||||
| case RecurKind::UMax: | ||||||||
| return match(Inc, m_UMax(m_Value(A), m_Value(B))); | ||||||||
| case RecurKind::UMin: | ||||||||
| return match(Inc, m_UMin(m_Value(A), m_Value(B))); | ||||||||
| case RecurKind::SMax: | ||||||||
| return match(Inc, m_SMax(m_Value(A), m_Value(B))); | ||||||||
| case RecurKind::SMin: | ||||||||
| return match(Inc, m_SMin(m_Value(A), m_Value(B))); | ||||||||
| default: | ||||||||
| llvm_unreachable("all min/max kinds must be handled"); | ||||||||
|
Collaborator
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.
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. This checks for completeness, all integer min/max should be handled, and unreachable here should make it slightly easier to catch missing cases together with the check above |
||||||||
| } | ||||||||
| }(); | ||||||||
| if (!IsMinMax) | ||||||||
| return false; | ||||||||
|
|
||||||||
| if (A == B || (A != Phi && B != Phi)) | ||||||||
|
Collaborator
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: can be written as
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 left it as-is for now, thanks |
||||||||
| return false; | ||||||||
|
Collaborator
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 checks that one of the multiple users of Phi is Inc, what about checking that another user is in the loop (rather than live-out)?
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. The other users will be checked in VPlan currently.
Collaborator
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. Ok, but the name of the function and its documentation specifically claim it has a user in the loop. Better rename Note that every reduction surely has users outside its chain, but they typically use the final post-updated value rather than the intermediate phi value. So
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. Ah yes, should be updated ,thanks |
||||||||
|
|
||||||||
| SmallPtrSet<Instruction *, 4> CastInsts; | ||||||||
| Value *RdxStart = Phi->getIncomingValueForBlock(TheLoop->getLoopPreheader()); | ||||||||
| RedDes = | ||||||||
| RecurrenceDescriptor(RdxStart, /*Exit=*/nullptr, /*Store=*/nullptr, Kind, | ||||||||
| FastMathFlags(), /*ExactFP=*/nullptr, Phi->getType(), | ||||||||
| /*Signed=*/false, /*Ordered=*/false, CastInsts, | ||||||||
| /*MinWidthCastToRecurTy=*/-1U, /*PhiMultiUse=*/true); | ||||||||
| return true; | ||||||||
| } | ||||||||
|
|
||||||||
| bool RecurrenceDescriptor::AddReductionVar( | ||||||||
| PHINode *Phi, RecurKind Kind, Loop *TheLoop, FastMathFlags FuncFMF, | ||||||||
| RecurrenceDescriptor &RedDes, DemandedBits *DB, AssumptionCache *AC, | ||||||||
|
|
@@ -227,6 +273,11 @@ bool RecurrenceDescriptor::AddReductionVar( | |||||||
| if (Phi->getParent() != TheLoop->getHeader()) | ||||||||
| return false; | ||||||||
|
|
||||||||
| // Check for min/max reduction variables that feed other users in the loop. | ||||||||
| if (isMinMaxReductionPhiWithUsersOutsideReductionChain(Phi, Kind, TheLoop, | ||||||||
| RedDes)) | ||||||||
| return true; | ||||||||
|
|
||||||||
| // Obtain the reduction start value from the value that comes from the loop | ||||||||
| // preheader. | ||||||||
| Value *RdxStart = Phi->getIncomingValueForBlock(TheLoop->getLoopPreheader()); | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6593,6 +6593,11 @@ void LoopVectorizationCostModel::collectInLoopReductions() { | |
| PHINode *Phi = Reduction.first; | ||
| const RecurrenceDescriptor &RdxDesc = Reduction.second; | ||
|
|
||
| // Multi-use reductions (e.g., used in FindLastIV patterns) are handled | ||
| // separately and should not be considered for in-loop reductions. | ||
| if (RdxDesc.hasUsesOutsideReductionChain()) | ||
| continue; | ||
|
|
||
| // We don't collect reductions that are type promoted (yet). | ||
| if (RdxDesc.getRecurrenceType() != Phi->getType()) | ||
| continue; | ||
|
|
@@ -7998,9 +8003,10 @@ void VPRecipeBuilder::collectScaledReductions(VFRange &Range) { | |
| MapVector<Instruction *, | ||
| SmallVector<std::pair<PartialReductionChain, unsigned>>> | ||
| ChainsByPhi; | ||
| for (const auto &[Phi, RdxDesc] : Legal->getReductionVars()) | ||
| getScaledReductions(Phi, RdxDesc.getLoopExitInstr(), Range, | ||
| ChainsByPhi[Phi]); | ||
| for (const auto &[Phi, RdxDesc] : Legal->getReductionVars()) { | ||
| if (Instruction *RdxExitInstr = RdxDesc.getLoopExitInstr()) | ||
| getScaledReductions(Phi, RdxExitInstr, Range, ChainsByPhi[Phi]); | ||
|
Comment on lines
+8007
to
+8008
Collaborator
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: may be good to wrap loop body in brackets.
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. done thanks |
||
| } | ||
|
|
||
| // A partial reduction is invalid if any of its extends are used by | ||
| // something that isn't another partial reduction. This is because the | ||
|
|
@@ -8221,7 +8227,8 @@ VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe(VPSingleDefRecipe *R, | |
| PhiRecipe = new VPReductionPHIRecipe( | ||
| Phi, RdxDesc.getRecurrenceKind(), *StartV, | ||
| getReductionStyle(UseInLoopReduction, UseOrderedReductions, | ||
| ScaleFactor)); | ||
| ScaleFactor), | ||
| RdxDesc.hasUsesOutsideReductionChain()); | ||
| } else { | ||
| // TODO: Currently fixed-order recurrences are modeled as chains of | ||
| // first-order recurrences. If there are no users of the intermediate | ||
|
|
@@ -8555,6 +8562,11 @@ VPlanPtr LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes( | |
| // Adjust the recipes for any inloop reductions. | ||
| adjustRecipesForReductions(Plan, RecipeBuilder, Range.Start); | ||
|
|
||
| // Apply mandatory transformation to handle reductions with multiple in-loop | ||
| // uses if possible, bail out otherwise. | ||
| if (!VPlanTransforms::runPass(VPlanTransforms::handleMultiUseReductions, | ||
| *Plan)) | ||
| return nullptr; | ||
| // Apply mandatory transformation to handle FP maxnum/minnum reduction with | ||
| // NaNs if possible, bail out otherwise. | ||
| if (!VPlanTransforms::runPass(VPlanTransforms::handleMaxMinNumReductions, | ||
|
|
||
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.
Maybe need to add assertion to check the Phi has 2 incoming values, one is from latch (already checked), and another is from preheader.
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