-
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 16 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 | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -214,6 +214,40 @@ static bool checkOrderedReduction(RecurKind Kind, Instruction *ExactFPMathInst, | |||||||
| return true; | ||||||||
| } | ||||||||
|
|
||||||||
| static std::optional<RecurrenceDescriptor> | ||||||||
|
||||||||
| getMultiUseMinMax(PHINode *Phi, RecurKind Kind, Loop *TheLoop) { | ||||||||
|
||||||||
| BasicBlock *Latch = TheLoop->getLoopLatch(); | ||||||||
|
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. Maybe need to add assertion to check the Phi has 2 incoming values, one is from latch (already checked), and another is from preheader.
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 |
||||||||
| if (!Latch) | ||||||||
| return std::nullopt; | ||||||||
| Value *Inc = Phi->getIncomingValueForBlock(Latch); | ||||||||
| RecurKind RK; | ||||||||
| if (Phi->hasOneUse() || | ||||||||
| !RecurrenceDescriptor::isIntMinMaxRecurrenceKind(Kind)) | ||||||||
|
||||||||
| if (Phi->hasOneUse() || | |
| !RecurrenceDescriptor::isIntMinMaxRecurrenceKind(Kind)) | |
| if (Phi->hasOneUse() || !Inc->hasOneUse()) |
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, but I retained the isIntMinMaxRecurrenceKind for now, to catch missing cases below
Outdated
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.
replace by unreachable, having early-exited above if !isIntMinMaxRecurrenceKind(Kind), or remove that check above?
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.
Above checks the requested RecurrenceKind, here we check the incoming value, but the code was a bit confusing and didn't account properly for the requested Kind. Updated to remove setting RK, and only use the appropriate matcher for Kind, 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.
Nit: can be written as !((A == Phi) ^ (B == Phi)) or ((A == Phi) == (B == Phi)), but probably clearer as written.
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 left it as-is for now, thanks
Outdated
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.
Could add /* arg name*/ for each nullptr and false?
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
Outdated
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.
As a simple early-exit case of AddReductionVar(), should getMultiUseMinMax() also return bool and receive a reference of RecurrenceDescriptor as parameter, rather than return an optional? Name could also be more aligned.
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.
Yep updated the arguments, return value and naming, thanks
Outdated
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.
Should ++NumCmpSelectPatternInst; be added?
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 so, this is just used locally to verify min/max patterns later on.
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1252,7 +1252,9 @@ llvm::canParallelizeReductionWhenUnrolling(PHINode &Phi, Loop *L, | |||||||||||||||
| RecurrenceDescriptor RdxDesc; | ||||||||||||||||
| if (!RecurrenceDescriptor::isReductionPHI(&Phi, L, RdxDesc, | ||||||||||||||||
| /*DemandedBits=*/nullptr, | ||||||||||||||||
| /*AC=*/nullptr, /*DT=*/nullptr, SE)) | ||||||||||||||||
| /*AC=*/nullptr, /*DT=*/nullptr, | ||||||||||||||||
| SE) || | ||||||||||||||||
| RdxDesc.isPhiMultiUse()) | ||||||||||||||||
|
||||||||||||||||
| /*AC=*/nullptr, /*DT=*/nullptr, | |
| SE) || | |
| RdxDesc.isPhiMultiUse()) | |
| /*AC=*/nullptr, /*DT=*/nullptr, SE)) | |
| return std::nullopt; | |
| if (RdxDesc.isPhiMultiUse()) | |
| return std::nullopt; |
clearer to check isPhiMultiUse() after isReductionPHI() returned true and initialized RdxDesc?
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 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -6533,6 +6533,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.isPhiMultiUse()) | ||||||||||||
| continue; | ||||||||||||
|
|
||||||||||||
| // We don't collect reductions that are type promoted (yet). | ||||||||||||
| if (RdxDesc.getRecurrenceType() != Phi->getType()) | ||||||||||||
| continue; | ||||||||||||
|
|
@@ -7184,6 +7189,9 @@ static void fixReductionScalarResumeWhenVectorizingEpilog( | |||||||||||
| Value *StartV = getStartValueFromReductionResult(EpiRedResult); | ||||||||||||
| Value *SentinelV = EpiRedResult->getOperand(2)->getLiveInIRValue(); | ||||||||||||
| using namespace llvm::PatternMatch; | ||||||||||||
| MainResumeValue = cast<VPInstruction>(EpiRedHeaderPhi->getStartValue()) | ||||||||||||
| ->getOperand(0) | ||||||||||||
| ->getUnderlyingValue(); | ||||||||||||
|
||||||||||||
| Value *Cmp, *OrigResumeV, *CmpOp; | ||||||||||||
| [[maybe_unused]] bool IsExpectedPattern = | ||||||||||||
| match(MainResumeValue, | ||||||||||||
|
|
@@ -7194,7 +7202,11 @@ static void fixReductionScalarResumeWhenVectorizingEpilog( | |||||||||||
| ((CmpOp == StartV && isGuaranteedNotToBeUndefOrPoison(CmpOp)))); | ||||||||||||
| assert(IsExpectedPattern && "Unexpected reduction resume pattern"); | ||||||||||||
| MainResumeValue = OrigResumeV; | ||||||||||||
| } else if (auto *VPI = | ||||||||||||
| dyn_cast<VPInstruction>(EpiRedHeaderPhi->getStartValue())) { | ||||||||||||
| MainResumeValue = VPI->getOperand(0)->getUnderlyingValue(); | ||||||||||||
|
||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| PHINode *MainResumePhi = cast<PHINode>(MainResumeValue); | ||||||||||||
|
|
||||||||||||
| // When fixing reductions in the epilogue loop we should already have | ||||||||||||
|
|
@@ -7906,6 +7918,9 @@ void VPRecipeBuilder::collectScaledReductions(VFRange &Range) { | |||||||||||
| SmallVector<std::pair<PartialReductionChain, unsigned>> | ||||||||||||
| PartialReductionChains; | ||||||||||||
| for (const auto &[Phi, RdxDesc] : Legal->getReductionVars()) { | ||||||||||||
| if (RecurrenceDescriptor::isMinMaxRecurrenceKind( | ||||||||||||
| RdxDesc.getRecurrenceKind())) | ||||||||||||
| continue; | ||||||||||||
|
||||||||||||
| getScaledReductions(Phi, RdxDesc.getLoopExitInstr(), Range, | ||||||||||||
| PartialReductionChains); | ||||||||||||
| } | ||||||||||||
|
|
@@ -8070,9 +8085,6 @@ VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe(VPSingleDefRecipe *R, | |||||||||||
| return Recipe; | ||||||||||||
|
|
||||||||||||
| VPHeaderPHIRecipe *PhiRecipe = nullptr; | ||||||||||||
| assert((Legal->isReductionVariable(Phi) || | ||||||||||||
| Legal->isFixedOrderRecurrence(Phi)) && | ||||||||||||
| "can only widen reductions and fixed-order recurrences here"); | ||||||||||||
| VPValue *StartV = Operands[0]; | ||||||||||||
| if (Legal->isReductionVariable(Phi)) { | ||||||||||||
| const RecurrenceDescriptor &RdxDesc = Legal->getRecurrenceDescriptor(Phi); | ||||||||||||
|
|
@@ -8084,13 +8096,19 @@ VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe(VPSingleDefRecipe *R, | |||||||||||
| getScalingForReduction(RdxDesc.getLoopExitInstr()).value_or(1); | ||||||||||||
| PhiRecipe = new VPReductionPHIRecipe( | ||||||||||||
| Phi, RdxDesc.getRecurrenceKind(), *StartV, CM.isInLoopReduction(Phi), | ||||||||||||
| CM.useOrderedReductions(RdxDesc), ScaleFactor); | ||||||||||||
| } else { | ||||||||||||
| CM.useOrderedReductions(RdxDesc), ScaleFactor, | ||||||||||||
| RdxDesc.isPhiMultiUse()); | ||||||||||||
| } else if (Legal->isFixedOrderRecurrence(Phi)) { | ||||||||||||
|
||||||||||||
| // TODO: Currently fixed-order recurrences are modeled as chains of | ||||||||||||
| // first-order recurrences. If there are no users of the intermediate | ||||||||||||
| // recurrences in the chain, the fixed order recurrence should be modeled | ||||||||||||
| // directly, enabling more efficient codegen. | ||||||||||||
| PhiRecipe = new VPFirstOrderRecurrencePHIRecipe(Phi, *StartV); | ||||||||||||
| } else { | ||||||||||||
| // Failed to identify phi as reduction or fixed-order recurrence. Keep the | ||||||||||||
| // original VPWidenPHIRecipe for now, to be legalized later if possible. | ||||||||||||
|
||||||||||||
| setRecipe(Phi, R); | ||||||||||||
| return nullptr; | ||||||||||||
|
||||||||||||
| } | ||||||||||||
| // Add backedge value. | ||||||||||||
| PhiRecipe->addOperand(Operands[1]); | ||||||||||||
|
|
@@ -8365,8 +8383,11 @@ VPlanPtr LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes( | |||||||||||
|
|
||||||||||||
| VPRecipeBase *Recipe = | ||||||||||||
| RecipeBuilder.tryToCreateWidenRecipe(SingleDef, Range); | ||||||||||||
| if (!Recipe) | ||||||||||||
| if (!Recipe) { | ||||||||||||
| if (isa<VPPhi>(SingleDef)) | ||||||||||||
| continue; | ||||||||||||
|
||||||||||||
| Recipe = RecipeBuilder.handleReplication(Instr, R.operands(), Range); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| RecipeBuilder.setRecipe(Instr, Recipe); | ||||||||||||
| if (isa<VPWidenIntOrFpInductionRecipe>(Recipe) && isa<TruncInst>(Instr)) { | ||||||||||||
|
|
@@ -8428,6 +8449,10 @@ VPlanPtr LoopVectorizationPlanner::tryToBuildVPlanWithVPRecipes( | |||||||||||
| // Adjust the recipes for any inloop reductions. | ||||||||||||
| adjustRecipesForReductions(Plan, RecipeBuilder, Range.Start); | ||||||||||||
|
|
||||||||||||
| // Try to legalize reductions with multiple in-loop uses. | ||||||||||||
| if (!VPlanTransforms::runPass(VPlanTransforms::legalizeMultiUseReductions, | ||||||||||||
|
||||||||||||
| // Try to legalize reductions with multiple in-loop uses. | |
| if (!VPlanTransforms::runPass(VPlanTransforms::legalizeMultiUseReductions, | |
| // Apply mandatory transformation to handle reductions with multiple in-loop | |
| // uses if possible, bail out otherwise. | |
| if (!VPlanTransforms::runPass(VPlanTransforms::handleMultiUseReductions, |
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 |
|---|---|---|
|
|
@@ -1994,7 +1994,8 @@ class LLVM_ABI_FOR_TEST VPHeaderPHIRecipe : public VPSingleDefRecipe, | |
| ~VPHeaderPHIRecipe() override = default; | ||
|
|
||
| /// Method to support type inquiry through isa, cast, and dyn_cast. | ||
| static inline bool classof(const VPRecipeBase *B) { | ||
| static inline bool classof(const VPUser *U) { | ||
| auto *B = cast<VPRecipeBase>(U); | ||
| return B->getVPDefID() >= VPDef::VPFirstHeaderPHISC && | ||
| B->getVPDefID() <= VPDef::VPLastHeaderPHISC; | ||
| } | ||
|
|
@@ -2003,6 +2004,10 @@ class LLVM_ABI_FOR_TEST VPHeaderPHIRecipe : public VPSingleDefRecipe, | |
| return B && B->getVPDefID() >= VPRecipeBase::VPFirstHeaderPHISC && | ||
| B->getVPDefID() <= VPRecipeBase::VPLastHeaderPHISC; | ||
| } | ||
| static inline bool classof(const VPSingleDefRecipe *B) { | ||
| return B->getVPDefID() >= VPDef::VPFirstHeaderPHISC && | ||
| B->getVPDefID() <= VPDef::VPLastHeaderPHISC; | ||
|
||
| } | ||
|
|
||
| /// Generate the phi nodes. | ||
| void execute(VPTransformState &State) override = 0; | ||
|
|
@@ -2067,7 +2072,7 @@ class VPWidenInductionRecipe : public VPHeaderPHIRecipe { | |
| return R && classof(R); | ||
| } | ||
|
|
||
| static inline bool classof(const VPHeaderPHIRecipe *R) { | ||
| static inline bool classof(const VPSingleDefRecipe *R) { | ||
| return classof(static_cast<const VPRecipeBase *>(R)); | ||
| } | ||
|
|
||
|
|
@@ -2344,6 +2349,10 @@ class VPReductionPHIRecipe : public VPHeaderPHIRecipe, | |
| /// The phi is part of an ordered reduction. Requires IsInLoop to be true. | ||
| bool IsOrdered; | ||
|
|
||
| /// The phi is part of a multi-use reduction (e.g., used in FindLastIV | ||
| /// patterns). | ||
|
||
| bool IsPhiMultiUse; | ||
|
||
|
|
||
| /// When expanding the reduction PHI, the plan's VF element count is divided | ||
| /// by this factor to form the reduction phi's VF. | ||
| unsigned VFScaleFactor = 1; | ||
|
|
@@ -2352,9 +2361,10 @@ class VPReductionPHIRecipe : public VPHeaderPHIRecipe, | |
| /// Create a new VPReductionPHIRecipe for the reduction \p Phi. | ||
| VPReductionPHIRecipe(PHINode *Phi, RecurKind Kind, VPValue &Start, | ||
| bool IsInLoop = false, bool IsOrdered = false, | ||
| unsigned VFScaleFactor = 1) | ||
| unsigned VFScaleFactor = 1, bool IsPhiMultiUse = false) | ||
| : VPHeaderPHIRecipe(VPDef::VPReductionPHISC, Phi, &Start), Kind(Kind), | ||
| IsInLoop(IsInLoop), IsOrdered(IsOrdered), VFScaleFactor(VFScaleFactor) { | ||
| IsInLoop(IsInLoop), IsOrdered(IsOrdered), IsPhiMultiUse(IsPhiMultiUse), | ||
| VFScaleFactor(VFScaleFactor) { | ||
| assert((!IsOrdered || IsInLoop) && "IsOrdered requires IsInLoop"); | ||
| } | ||
|
|
||
|
|
@@ -2363,7 +2373,7 @@ class VPReductionPHIRecipe : public VPHeaderPHIRecipe, | |
| VPReductionPHIRecipe *clone() override { | ||
| auto *R = new VPReductionPHIRecipe( | ||
| dyn_cast_or_null<PHINode>(getUnderlyingValue()), getRecurrenceKind(), | ||
| *getOperand(0), IsInLoop, IsOrdered, VFScaleFactor); | ||
| *getOperand(0), IsInLoop, IsOrdered, VFScaleFactor, IsPhiMultiUse); | ||
| R->addOperand(getBackedgeValue()); | ||
| return R; | ||
| } | ||
|
|
@@ -2396,6 +2406,9 @@ class VPReductionPHIRecipe : public VPHeaderPHIRecipe, | |
| /// Returns true, if the phi is part of an in-loop reduction. | ||
| bool isInLoop() const { return IsInLoop; } | ||
|
|
||
| /// Returns true, if the phi is part of a multi-use reduction. | ||
| bool isPhiMultiUse() const { return IsPhiMultiUse; } | ||
|
|
||
| /// Returns true if the recipe only uses the first lane of operand \p Op. | ||
| bool onlyFirstLaneUsed(const VPValue *Op) const override { | ||
| assert(is_contained(operands(), Op) && | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -948,3 +948,101 @@ bool VPlanTransforms::handleMaxMinNumReductions(VPlan &Plan) { | |||||||
| MiddleTerm->setOperand(0, NewCond); | ||||||||
| return true; | ||||||||
| } | ||||||||
|
|
||||||||
| bool VPlanTransforms::legalizeMultiUseReductions(VPlan &Plan) { | ||||||||
| for (auto &PhiR : make_early_inc_range( | ||||||||
| Plan.getVectorLoopRegion()->getEntryBasicBlock()->phis())) { | ||||||||
| auto *MinMaxPhiR = dyn_cast<VPReductionPHIRecipe>(&PhiR); | ||||||||
| if (!MinMaxPhiR) | ||||||||
| continue; | ||||||||
|
|
||||||||
| RecurKind RdxKind = MinMaxPhiR->getRecurrenceKind(); | ||||||||
| // TODO: check for multi-uses in VPlan directly. | ||||||||
| if (!RecurrenceDescriptor::isIntMinMaxRecurrenceKind(RdxKind) || | ||||||||
| !MinMaxPhiR->isPhiMultiUse()) | ||||||||
|
||||||||
| continue; | ||||||||
|
|
||||||||
| // One user of MinMaxPhiR is MinMaxOp, the other users must be a compare | ||||||||
|
||||||||
| // One user of MinMaxPhiR is MinMaxOp, the other users must be a compare | |
| // One user of MinMaxPhiR is MinMaxOp, the other user must be a compare |
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
Outdated
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.
MultiUse can be more accurately DoubleUse.
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.
Yep, although I am not sure if we should tighten this down early on? A single min/max could serve multiple selects, supported here eventually?
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.
Multiple selects could conceptually be fed by a single min/max, but are there conceivable use cases? The core argmin/argmax pattern seems sufficiently prevalent and involved to recognize and handle, to arguably deserve specific attention.
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.
yep, now the multi use terminology has been replaced, would you prefer to make the initial check more specific?
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.
Better clarify what patterns are supported as accurately as possible at the outset.
Somewhat confusing - isMinMaxReductionWithLoopUsersOutsideReductionChain() checks that Phi has more than one user (in the loop) and Inc has one - here MinMaxOp (aka Inc?) has two? Should MinMaxOp be checked to have 2 operands but (asserted to have) 1 user?
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.
Thanks, I added a comment after the initial early continue in the loop.
I added the assert (and more asserts later below checking the expected users)
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.
| return false; | |
| return false; | |
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.
added ,thanks
Outdated
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.
Should check the number of phi's users, like #141467 did:
// TODO: support min/max with 2-D indexes.
if (!Phi->hasNUses(2))
return false;
Outdated
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.
Could this use some form of findUser[W/ or W/O Opcode]() as in findComputeReductionResult()?
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.
Probably yes, but it get's a bit more complicated, as the compare could be different recipes (replicate/widen)
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.
Could find_if(users, != MinMaxOp) be used, as in findReductionUser(), for consistency?
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.
updated to use new findUser with pattern matcher
Outdated
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.
compress into one line, and choose the better name.
Outdated
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.
So that argmax corresponds to GE/GT and argmin corresponds to LE/LT (FindLastIV/FindFirstIV for both. respectively)
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.
Yep
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.
Bail out now (regardless of RdxKind) if Pred is strict as only FindLastIV is supported?
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.
Yep done, thanks!
Outdated
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.
Is it important to normalize so MinMaxPhiR is on the right - Pred appears to be unused?
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.
Not for the current version, but will be needed later to support more cases. I removed it for now, thanks
Outdated
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.
| // Account for a mis-match between RdxKind and the predicate. | |
| // Account for a mismatch between RdxKind and the predicate. |
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.
This whole switch is now gone, thanks
Outdated
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.
Bail out now if Pred isGE - matching (Last) argmax, mismatching RdxKind UMin/Smin?
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.
Covered by earlier bail out, remove switch, thanks
Outdated
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.
Bail out now if Pred isLE - matching (Last) argmin, mismatching RdxKind UMax/Smax?
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.
Covered by earlier bail out, remove switch, thanks
Outdated
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.
Can bail out if Pred is NE/EQ earlier.
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.
Bailed out earlier, 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.
Worth asserting that MinMaxOp aligns with Pred, i.e., both compute max or both compute min?
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.
yep this is now checked below, thanks
Outdated
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.
| VPValue *Sel = dyn_cast<VPSingleDefRecipe>(*Cmp->user_begin()); | |
| auto *Sel = dyn_cast<VPSingleDefRecipe>(*Cmp->user_begin()); |
Outdated
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.
Could IVOp be scalar/derived?
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.
Not yet at this point, as scalar-steps are introduced as optimization later
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.
| return false; | |
| return false; | |
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.
added 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.
If FindIVPhiR has a FindLastIVRecurrenceKind, does that imply IVOp must be a VPWidenIntOrFpInductionRecipe?
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.
yes, moved check from above to assert below, thanks
Outdated
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.
May be helpful to show snippets of VPInstructions before and after these steps.
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!
Outdated
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.
Is there a clearer and consistent way to "get the other user" among two users of a VPValue, as done above when setting Cmp above to the user of MinMaxPhiR other than MinMaxOp (using to_vector etc.) and here setting FindIVResult to the user of Sel other than FindIVPhiR?
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 updated the code to use the common findComputeReductionResult
Outdated
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.
Would be good to explicitly Broadcast MinMaxResult before feeding it to compare with its operand(1)?
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.
At this stage, the broadcasts are still implicit and will introduced explicitly at a later stage.
Outdated
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.
Worth assigning variables with meaningful names to the above operand(1) and these operand(2),(3), especially in the absence of documenting these VPInstruction opcodes.
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.
I think we can just call it isReduction?
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.
The reason I went with specically the multi-use naming is that even with multiple uses of the phi, the phi ifself is still a reduction, although the multiple users prevent (or make more difficult) transformations. WDYT?
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.
Perhaps
IsUsedInLooporIsPhiReusedInLoopwould be more accurate thanIsPhiMultiUse. The phi is clearly used once in the loop being part of a recurrence, this indicates that it has another use in the loop - as opposed to being live-out.An argmax/argmin pattern where a min/max reduction feeds a FindLastIV reduction may be indicated with something like
IsFeedingAnotherReduction- as opposed to feeding say a non-reducing in-loop store (cf. a cumulative-sum pattern where a sum reduction feeds a non-reducing in-loop store). But here the additional in-loop user(s) has yet to be analyzed if it's reducing or not?An argmax/argmin pattern conceptually has multiple users of it's compare condition rather than of its phi, as in:
But if the argmax/argmin pattern uses a max/min intrinsic instead of a compare-select pair, the resultMax phi will have multiple users instead of the updateMax compare:
Would be good to explain, and support both (follow-up)?
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.
That sounds good, but I think "IsMultiUse" would be enough. If there is more than one operation in the recurrence chain, i.e., phi-> max -> max, not only phi will have user, but max in the middle will also have user.
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.
However, this doesn't seem precise enough. If it's a cmp-select form of min/max, it's normal for phi to have more than one user. It should more accurately refer to multiple users outside the recurrence chain.
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 updated it to the more verbose
PhiHasLoopUsesOutsideReductionChain. WDYT?Exactly, we delay checking the other users here, as it naturally will be handled by the existing analysis, and matching/legalization can easily be done in VPlan.