Skip to content

[LV] Keep duplicate recipes in VPExpressionRecipe #156976

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 7 commits into from
Oct 1, 2025
Merged

[LV] Keep duplicate recipes in VPExpressionRecipe #156976

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7328e0d
[LV] Keep duplicate recipes in VPExpressionRecipe
SamTebbs33 Sep 4, 2025
d2fe632
remove underlying value set
SamTebbs33 Sep 19, 2025
0834362
Cache placeholder values
SamTebbs33 Sep 23, 2025
73559dd
Address review
SamTebbs33 Sep 26, 2025
1ae4d37
Use SmallPtrSet and remove placeholder map
SamTebbs33 Sep 30, 2025
1d6e918
Rebase and add comment about external operand replacement
SamTebbs33 Oct 1, 2025
40f27bb
Remove extra test changes and simplify constructor call
SamTebbs33 Oct 1, 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
11 changes: 8 additions & 3 deletions llvm/lib/Transforms/Vectorize/VPlan.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/SmallBitVector.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/Twine.h"
#include "llvm/ADT/ilist.h"
Expand Down Expand Up @@ -2977,7 +2978,8 @@ class LLVM_ABI_FOR_TEST VPBranchOnMaskRecipe : public VPRecipeBase {
/// the expression is elevated to connect the non-expression recipe with the
/// VPExpressionRecipe itself.
class VPExpressionRecipe : public VPSingleDefRecipe {
/// Recipes included in this VPExpressionRecipe.
/// Recipes included in this VPExpressionRecipe. This could contain
/// duplicates.
SmallVector<VPSingleDefRecipe *> ExpressionRecipes;

/// Temporary VPValues used for external operands of the expression, i.e.
Expand Down Expand Up @@ -3039,8 +3041,11 @@ class VPExpressionRecipe : public VPSingleDefRecipe {
}

~VPExpressionRecipe() override {
for (auto *R : reverse(ExpressionRecipes))
delete R;
SmallPtrSet<VPSingleDefRecipe *, 4> ExpressionRecipesSeen;
for (auto *R : reverse(ExpressionRecipes)) {
if (ExpressionRecipesSeen.insert(R).second)
Copy link
Collaborator

@sdesmalen-arm sdesmalen-arm Sep 22, 2025

Choose a reason for hiding this comment

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

Can you update the comment for SmallVector<VPSingleDefRecipe *> ExpressionRecipes (line 2973) that this vector may contain duplicates?

Copy link
Collaborator Author

@SamTebbs33 SamTebbs33 Sep 23, 2025

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

@sdesmalen-arm sdesmalen-arm Sep 26, 2025

Choose a reason for hiding this comment

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

This is not done?

Copy link
Collaborator Author

@SamTebbs33 SamTebbs33 Sep 26, 2025

Choose a reason for hiding this comment

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

Sorry, I did add it but must not have staged it.

delete R;
}
for (VPValue *T : LiveInPlaceholders)
delete T;
}
Expand Down
17 changes: 12 additions & 5 deletions llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2755,9 +2755,8 @@ VPExpressionRecipe::VPExpressionRecipe(
ExpressionTypes ExpressionType,
ArrayRef<VPSingleDefRecipe *> ExpressionRecipes)
: VPSingleDefRecipe(VPDef::VPExpressionSC, {}, {}),
ExpressionRecipes(SetVector<VPSingleDefRecipe *>(
ExpressionRecipes.begin(), ExpressionRecipes.end())
.takeVector()),
ExpressionRecipes(SmallVector<VPSingleDefRecipe *>(
ExpressionRecipes.begin(), ExpressionRecipes.end())),
Copy link
Contributor

@fhahn fhahn Oct 1, 2025

Choose a reason for hiding this comment

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

Suggested change
ExpressionRecipes(SmallVector<VPSingleDefRecipe *>(
ExpressionRecipes.begin(), ExpressionRecipes.end())),
ExpressionRecipes(ExpressionRecipes),

Does this now work? I think the explicit constructor was only needed to de-duplicate

Copy link
Collaborator Author

@SamTebbs33 SamTebbs33 Oct 1, 2025

Choose a reason for hiding this comment

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

Done, thank you.

ExpressionType(ExpressionType) {
assert(!ExpressionRecipes.empty() && "Nothing to combine?");
assert(
Expand Down Expand Up @@ -2802,14 +2801,22 @@ VPExpressionRecipe::VPExpressionRecipe(
continue;
addOperand(Op);
LiveInPlaceholders.push_back(new VPValue());
R->setOperand(Idx, LiveInPlaceholders.back());
}
}

// Replace each external operand with the first one created for it in
// LiveInPlaceholders.
for (auto *R : ExpressionRecipes)
Copy link
Contributor

@fhahn fhahn Sep 30, 2025

Choose a reason for hiding this comment

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

This subtly changes the behavior; we could have the same external operand used multiple times, and we will have multiple entries for them in LiveInPlaceholders, but only use the first of them.

I'm not sure if there's a way around this inconsistency?

Copy link
Collaborator

@sdesmalen-arm sdesmalen-arm Sep 30, 2025

Choose a reason for hiding this comment

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

If there are two placeholders for a value where only one of them is used when decomposing the expression, what is the practical issue with that?

Copy link
Contributor

@fhahn fhahn Oct 1, 2025

Choose a reason for hiding this comment

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

I don't think it is a practical issue at the moment, but as mentioned in #156976 (review) it should at least be mentioned/documented.

Or perhaps even better also store a map of live-ins and their tmp values, instead of a vector and creating unused temporary values.

Copy link
Collaborator Author

@SamTebbs33 SamTebbs33 Oct 1, 2025

Choose a reason for hiding this comment

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

The latest commit has actually eliminated this behavioural change so I don't think we need to document anything, but I can do that as a follow-up if that changes.

Copy link
Contributor

@fhahn fhahn Oct 1, 2025

Choose a reason for hiding this comment

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

doing replace-uses-with will update all uses of the external operand with the first temporary value we created for it, leaving the other tmp values created for the same external operand unused

Copy link
Collaborator Author

@SamTebbs33 SamTebbs33 Oct 1, 2025

Choose a reason for hiding this comment

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

Ah thanks, I missed that. I've added a comment as part of a rebase.

for (auto const &[LiveIn, Tmp] : zip(operands(), LiveInPlaceholders))
R->replaceUsesOfWith(LiveIn, Tmp);
}

void VPExpressionRecipe::decompose() {
for (auto *R : ExpressionRecipes)
R->insertBefore(this);
// Since the list could contain duplicates, make sure the recipe hasn't
// already been inserted.
if (!R->getParent())
Copy link
Collaborator

@sdesmalen-arm sdesmalen-arm Sep 26, 2025

Choose a reason for hiding this comment

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

This needs a comment explaining why the !R->getParent().

Copy link
Collaborator Author

@SamTebbs33 SamTebbs33 Sep 26, 2025

Choose a reason for hiding this comment

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

Done, thanks.

R->insertBefore(this);

for (const auto &[Idx, Op] : enumerate(operands()))
LiveInPlaceholders[Idx]->replaceAllUsesWith(Op);
Expand Down
Loading