Skip to content

[VPlan] Replace RdxDesc with RecurKind in VPReductionPHIRecipe (NFC). #142322

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

Merged
merged 8 commits into from
Jul 6, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,11 @@ class LoopVectorizationLegality {
/// Returns the reduction variables found in the loop.
const ReductionList &getReductionVars() const { return Reductions; }

RecurrenceDescriptor getRecurrenceDescriptor(PHINode *PN) const {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
RecurrenceDescriptor getRecurrenceDescriptor(PHINode *PN) const {
/// Returns the recurrence descriptor associated with a given phi node \p PN,
/// expecting one to exist.
RecurrenceDescriptor getRecurrenceDescriptor(PHINode *PN) const {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in split off c5fff13, thanks

assert(Reductions.contains(PN) && "no recurrence descriptor for phi");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert(Reductions.contains(PN) && "no recurrence descriptor for phi");
assert(isReductionVariable(PN) && "only reductions have recurrence descriptors");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in split off c5fff13, thanks

return Reductions.lookup(PN);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent: can search and replace several usages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in split off c5fff13, thanks

}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent: can update below
bool isReductionVariable(PHINode *PN) const { return Reductions.count(PN); }
to
bool isReductionVariable(PHINode *PN) const { return Reductions.contains(PN); }

/// Returns the induction variables found in the loop.
const InductionList &getInductionVars() const { return Inductions; }

Expand Down
29 changes: 10 additions & 19 deletions llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7267,8 +7267,7 @@ static void fixReductionScalarResumeWhenVectorizingEpilog(

auto *EpiRedHeaderPhi =
cast<VPReductionPHIRecipe>(EpiRedResult->getOperand(0));
const RecurrenceDescriptor &RdxDesc =
EpiRedHeaderPhi->getRecurrenceDescriptor();
RecurKind Kind = EpiRedHeaderPhi->getRecurrenceKind();
Value *MainResumeValue;
if (auto *VPI = dyn_cast<VPInstruction>(EpiRedHeaderPhi->getStartValue())) {
assert((VPI->getOpcode() == VPInstruction::Broadcast ||
Expand All @@ -7277,8 +7276,7 @@ static void fixReductionScalarResumeWhenVectorizingEpilog(
MainResumeValue = VPI->getOperand(0)->getUnderlyingValue();
} else
MainResumeValue = EpiRedHeaderPhi->getStartValue()->getUnderlyingValue();
if (RecurrenceDescriptor::isAnyOfRecurrenceKind(
RdxDesc.getRecurrenceKind())) {
if (RecurrenceDescriptor::isAnyOfRecurrenceKind(Kind)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent: can shorten to isAnyOf(), isFindIV(), etc., given that the parameter is a Recur[rence]Kind.

[[maybe_unused]] Value *StartV =
EpiRedResult->getOperand(1)->getLiveInIRValue();
auto *Cmp = cast<ICmpInst>(MainResumeValue);
Expand All @@ -7288,8 +7286,7 @@ static void fixReductionScalarResumeWhenVectorizingEpilog(
"AnyOf expected to start by comparing main resume value to original "
"start value");
MainResumeValue = Cmp->getOperand(0);
} else if (RecurrenceDescriptor::isFindIVRecurrenceKind(
RdxDesc.getRecurrenceKind())) {
} else if (RecurrenceDescriptor::isFindIVRecurrenceKind(Kind)) {
Value *StartV = getStartValueFromReductionResult(EpiRedResult);
Value *SentinelV = EpiRedResult->getOperand(2)->getLiveInIRValue();
using namespace llvm::PatternMatch;
Expand Down Expand Up @@ -8310,7 +8307,7 @@ VPRecipeBase *VPRecipeBuilder::tryToCreateWidenRecipe(VPSingleDefRecipe *R,
unsigned ScaleFactor =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above

      const RecurrenceDescriptor &RdxDesc =
          Legal->getReductionVars().find(Phi)->second;

can be replaced (independently) with

      const RecurrenceDescriptor &RdxDesc =
          Legal->getRecurrenceDescriptor(Phi);

?

Similar candidates in
LoopVectorizationCostModel::collectElementTypesForWidening(), LoopVectorizationCostModel::getReductionPatternCost().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in c5fff13, thanks

getScalingForReduction(RdxDesc.getLoopExitInstr()).value_or(1);
PhiRecipe = new VPReductionPHIRecipe(
Phi, RdxDesc, *StartV, CM.isInLoopReduction(Phi),
Phi, RdxDesc.getRecurrenceKind(), *StartV, CM.isInLoopReduction(Phi),
CM.useOrderedReductions(RdxDesc), ScaleFactor);
} else {
// TODO: Currently fixed-order recurrences are modeled as chains of
Expand Down Expand Up @@ -9070,8 +9067,7 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
if (!PhiR || !PhiR->isInLoop() || (MinVF.isScalar() && !PhiR->isOrdered()))
continue;

const RecurrenceDescriptor &RdxDesc = PhiR->getRecurrenceDescriptor();
RecurKind Kind = RdxDesc.getRecurrenceKind();
RecurKind Kind = PhiR->getRecurrenceKind();
assert(
!RecurrenceDescriptor::isAnyOfRecurrenceKind(Kind) &&
!RecurrenceDescriptor::isFindIVRecurrenceKind(Kind) &&
Expand Down Expand Up @@ -9177,6 +9173,8 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
if (CM.blockNeedsPredicationForAnyReason(CurrentLinkI->getParent()))
CondOp = RecipeBuilder.getBlockInMask(CurrentLink->getParent());

RecurrenceDescriptor RdxDesc = Legal->getRecurrenceDescriptor(
cast<PHINode>(PhiR->getUnderlyingInstr()));
Comment on lines +9175 to +9176
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO to also capture RdxDesc's FMF in PhiR, similar to capturing its Kind, to avoid retrieving RdxDesc here?
Can CM.useOrderedReductions(RdxDesc) below be replaced with PhiR->isOrdered()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a TODO, thanks

// Non-FP RdxDescs will have all fast math flags set, so clear them.
FastMathFlags FMFs = isa<FPMathOperator>(CurrentLinkI)
? RdxDesc.getFastMathFlags()
Expand Down Expand Up @@ -9207,7 +9205,8 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
if (!PhiR)
continue;

const RecurrenceDescriptor &RdxDesc = PhiR->getRecurrenceDescriptor();
const RecurrenceDescriptor &RdxDesc = Legal->getRecurrenceDescriptor(
cast<PHINode>(PhiR->getUnderlyingInstr()));
Comment on lines +9207 to +9208
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const RecurrenceDescriptor &RdxDesc = Legal->getRecurrenceDescriptor(
cast<PHINode>(PhiR->getUnderlyingInstr()));
RecurKind RecurrenceKind = PhiR->getRecurrenceKind();
const RecurrenceDescriptor &RdxDesc = Legal->getRecurrenceDescriptor(
cast<PHINode>(PhiR->getUnderlyingInstr()));

and replace RdxDesc.getRecurrenceKind() below with RecurrenceKind?

Getting rid of RdxDesc here is more difficult: in addition to capturing RdxDesc's FMF in PhiR, code below also uses RdxDesc to retrieve sentinel value, recurrence type, isSigned, Loop Exit Instr, ... ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done to use Kind, thanks!

Type *PhiTy = PhiR->getUnderlyingValue()->getType();
// If tail is folded by masking, introduce selects between the phi
// and the users outside the vector region of each reduction, at the
Expand Down Expand Up @@ -9853,14 +9852,9 @@ preparePlanForEpilogueVectorLoop(VPlan &Plan, Loop *L,
}));
ResumeV = cast<PHINode>(ReductionPhi->getUnderlyingInstr())
->getIncomingValueForBlock(L->getLoopPreheader());
const RecurrenceDescriptor &RdxDesc =
ReductionPhi->getRecurrenceDescriptor();
RecurKind RK = RdxDesc.getRecurrenceKind();
RecurKind RK = ReductionPhi->getRecurrenceKind();
if (RecurrenceDescriptor::isAnyOfRecurrenceKind(RK)) {
Value *StartV = RdxResult->getOperand(1)->getLiveInIRValue();
assert(RdxDesc.getRecurrenceStartValue() == StartV &&
"start value from ComputeAnyOfResult must match");

// VPReductionPHIRecipes for AnyOf reductions expect a boolean as
// start value; compare the final value from the main vector loop
// to the start value.
Expand All @@ -9869,9 +9863,6 @@ preparePlanForEpilogueVectorLoop(VPlan &Plan, Loop *L,
ResumeV = Builder.CreateICmpNE(ResumeV, StartV);
} else if (RecurrenceDescriptor::isFindIVRecurrenceKind(RK)) {
Value *StartV = getStartValueFromReductionResult(RdxResult);
assert(RdxDesc.getRecurrenceStartValue() == StartV &&
"start value from ComputeFinIVResult must match");

ToFrozen[StartV] = cast<PHINode>(ResumeV)->getIncomingValueForBlock(
EPI.MainLoopIterationCountCheck);

Expand Down
22 changes: 10 additions & 12 deletions llvm/lib/Transforms/Vectorize/VPlan.h
Original file line number Diff line number Diff line change
Expand Up @@ -2192,7 +2192,7 @@ struct VPFirstOrderRecurrencePHIRecipe : public VPHeaderPHIRecipe {
class VPReductionPHIRecipe : public VPHeaderPHIRecipe,
public VPUnrollPartAccessor<2> {
/// Descriptor for the reduction.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Descriptor for the reduction.
/// The recurrence kind of the reduction.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed thanks

const RecurrenceDescriptor &RdxDesc;
const RecurKind Kind;

/// The phi is part of an in-loop reduction.
bool IsInLoop;
Expand All @@ -2206,21 +2206,21 @@ class VPReductionPHIRecipe : public VPHeaderPHIRecipe,

public:
/// Create a new VPReductionPHIRecipe for the reduction \p Phi described by \p
/// RdxDesc.
VPReductionPHIRecipe(PHINode *Phi, const RecurrenceDescriptor &RdxDesc,
VPValue &Start, bool IsInLoop = false,
bool IsOrdered = false, unsigned VFScaleFactor = 1)
: VPHeaderPHIRecipe(VPDef::VPReductionPHISC, Phi, &Start),
RdxDesc(RdxDesc), IsInLoop(IsInLoop), IsOrdered(IsOrdered),
VFScaleFactor(VFScaleFactor) {
/// Kind.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  /// Create a new VPReductionPHIRecipe for the reduction \p Phi according to
  /// several parameters.

(Kind alone does not fully "describe" the reduction, as RdxDesc did or tried to).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trimmed the description, thanks

VPReductionPHIRecipe(PHINode *Phi, RecurKind Kind, VPValue &Start,
bool IsInLoop = false, bool IsOrdered = false,
unsigned VFScaleFactor = 1)
: VPHeaderPHIRecipe(VPDef::VPReductionPHISC, Phi, &Start), Kind(Kind),
IsInLoop(IsInLoop), IsOrdered(IsOrdered), VFScaleFactor(VFScaleFactor) {
assert((!IsOrdered || IsInLoop) && "IsOrdered requires IsInLoop");
}

~VPReductionPHIRecipe() override = default;

VPReductionPHIRecipe *clone() override {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stripped thanks

auto *R = new VPReductionPHIRecipe(
dyn_cast_or_null<PHINode>(getUnderlyingValue()), RdxDesc,
dyn_cast_or_null<PHINode>(getUnderlyingValue()), getRecurrenceKind(),
*getOperand(0), IsInLoop, IsOrdered, VFScaleFactor);
R->addOperand(getBackedgeValue());
return R;
Expand All @@ -2240,9 +2240,7 @@ class VPReductionPHIRecipe : public VPHeaderPHIRecipe,
VPSlotTracker &SlotTracker) const override;
#endif

const RecurrenceDescriptor &getRecurrenceDescriptor() const {
return RdxDesc;
}
RecurKind getRecurrenceKind() const { return Kind; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
RecurKind getRecurrenceKind() const { return Kind; }
/// Returns the recurrence kind of the reduction.
RecurKind getRecurrenceKind() const { return Kind; }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added thanks


/// Returns true, if the phi is part of an ordered reduction.
bool isOrdered() const { return IsOrdered; }
Expand Down
16 changes: 7 additions & 9 deletions llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -740,8 +740,8 @@ Value *VPInstruction::generate(VPTransformState &State) {
Value *ReducedPartRdx = State.get(getOperand(2));
for (unsigned Idx = 3; Idx < getNumOperands(); ++Idx)
ReducedPartRdx = Builder.CreateBinOp(
(Instruction::BinaryOps)RecurrenceDescriptor::getOpcode(
RecurKind::AnyOf),
static_cast<Instruction::BinaryOps>(
RecurrenceDescriptor::getOpcode(RecurKind::AnyOf)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be done independently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good , will do separately, thanks

State.get(getOperand(Idx)), ReducedPartRdx, "bin.rdx");
return createAnyOfReduction(Builder, ReducedPartRdx,
State.get(getOperand(1), VPLane(0)), OrigPhi);
Expand All @@ -751,8 +751,7 @@ Value *VPInstruction::generate(VPTransformState &State) {
// and will be removed by breaking up the recipe further.
auto *PhiR = cast<VPReductionPHIRecipe>(getOperand(0));
// Get its reduction variable descriptor.
const RecurrenceDescriptor &RdxDesc = PhiR->getRecurrenceDescriptor();
RecurKind RK = RdxDesc.getRecurrenceKind();
[[maybe_unused]] RecurKind RK = PhiR->getRecurrenceKind();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[[maybe_unused]] RecurKind RK = PhiR->getRecurrenceKind();
RecurKind RK = PhiR->getRecurrenceKind();

current RK is set w/o [[maybe_unused]], surely used to set IsSigned below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep dropped, thanks

assert(RecurrenceDescriptor::isFindIVRecurrenceKind(RK) &&
"Unexpected reduction kind");
assert(!PhiR->isInLoop() &&
Expand Down Expand Up @@ -786,9 +785,8 @@ Value *VPInstruction::generate(VPTransformState &State) {
// and will be removed by breaking up the recipe further.
auto *PhiR = cast<VPReductionPHIRecipe>(getOperand(0));
// Get its reduction variable descriptor.
const RecurrenceDescriptor &RdxDesc = PhiR->getRecurrenceDescriptor();

RecurKind RK = RdxDesc.getRecurrenceKind();
RecurKind RK = PhiR->getRecurrenceKind();
assert(!RecurrenceDescriptor::isFindIVRecurrenceKind(RK) &&
"should be handled by ComputeFindIVResult");

Expand All @@ -814,9 +812,9 @@ Value *VPInstruction::generate(VPTransformState &State) {
if (RecurrenceDescriptor::isMinMaxRecurrenceKind(RK))
ReducedPartRdx = createMinMaxOp(Builder, RK, ReducedPartRdx, RdxPart);
else
ReducedPartRdx =
Builder.CreateBinOp((Instruction::BinaryOps)RdxDesc.getOpcode(),
RdxPart, ReducedPartRdx, "bin.rdx");
ReducedPartRdx = Builder.CreateBinOp(
(Instruction::BinaryOps)RecurrenceDescriptor::getOpcode(RK),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change this C-style cast to a static_cast?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated thanks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, can be done independently, keeping this patch focused on the RdxDesc.getOpcode() --> getOpcode(RK) transition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do separately, thanks

RdxPart, ReducedPartRdx, "bin.rdx");
}
}

Expand Down
3 changes: 1 addition & 2 deletions llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1735,8 +1735,7 @@ void VPlanTransforms::clearReductionWrapFlags(VPlan &Plan) {
auto *PhiR = dyn_cast<VPReductionPHIRecipe>(&R);
if (!PhiR)
continue;
const RecurrenceDescriptor &RdxDesc = PhiR->getRecurrenceDescriptor();
RecurKind RK = RdxDesc.getRecurrenceKind();
RecurKind RK = PhiR->getRecurrenceKind();
if (RK != RecurKind::Add && RK != RecurKind::Mul)
continue;

Expand Down
Loading