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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bc7024c
[VPlan] Replace RdxDesc with RecurKind in VPReductionPHIRecipe (NFC).
fhahn May 30, 2025
5d0c744
!fixup use static cast, add helper.
fhahn Jul 2, 2025
dca0d4b
!fixup unify constructors, use helper
fhahn Jul 3, 2025
4c2e494
Merge remote-tracking branch 'origin/main' into vplan-replace-rdxdesc…
fhahn Jul 6, 2025
0ab71f6
Merge remote-tracking branch 'origin/main' into vplan-replace-rdxdesc…
fhahn Jul 6, 2025
cb4b75d
!fixup address comments, thanks
fhahn Jul 6, 2025
db65c6f
Merge remote-tracking branch 'origin/main' into vplan-replace-rdxdesc…
fhahn Jul 6, 2025
0bd4d2a
!fixup remove unrelated change
fhahn Jul 6, 2025
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
Prev Previous commit
Next Next commit
!fixup use static cast, add helper.
  • Loading branch information
@fhahn
fhahn committed Jul 3, 2025
commit 5d0c744a3dcb51c80c0f03df1c497372b67861bb
5 changes: 5 additions & 0 deletions llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h
View file
Open in desktop
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
2 changes: 1 addition & 1 deletion llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9173,7 +9173,7 @@ void LoopVectorizationPlanner::adjustRecipesForReductions(
if (CM.blockNeedsPredicationForAnyReason(CurrentLinkI->getParent()))
CondOp = RecipeBuilder.getBlockInMask(CurrentLink->getParent());

const RecurrenceDescriptor &RdxDesc = Legal->getReductionVars().lookup(
RecurrenceDescriptor RdxDesc = Legal->getRecurrenceDescriptor(
cast<PHINode>(PhiR->getUnderlyingInstr()));
// Non-FP RdxDescs will have all fast math flags set, so clear them.
FastMathFlags FMFs = isa<FPMathOperator>(CurrentLinkI)
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
View file
Open in desktop
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 Down