-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[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.
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
7328e0d
d2fe632
0834362
73559dd
1ae4d37
1d6e918
40f27bb
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 |
|---|---|---|
|
|
@@ -2755,10 +2755,7 @@ VPExpressionRecipe::VPExpressionRecipe( | |
| ExpressionTypes ExpressionType, | ||
| ArrayRef<VPSingleDefRecipe *> ExpressionRecipes) | ||
| : VPSingleDefRecipe(VPDef::VPExpressionSC, {}, {}), | ||
| ExpressionRecipes(SetVector<VPSingleDefRecipe *>( | ||
| ExpressionRecipes.begin(), ExpressionRecipes.end()) | ||
| .takeVector()), | ||
| ExpressionType(ExpressionType) { | ||
| ExpressionRecipes(ExpressionRecipes), ExpressionType(ExpressionType) { | ||
| assert(!ExpressionRecipes.empty() && "Nothing to combine?"); | ||
| assert( | ||
| none_of(ExpressionRecipes, | ||
|
|
@@ -2802,14 +2799,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) | ||
|
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 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? 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. 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? 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 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. 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 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. 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. 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 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 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()) | ||
|
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 needs a comment explaining why the 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. |
||
| R->insertBefore(this); | ||
|
|
||
| for (const auto &[Idx, Op] : enumerate(operands())) | ||
| LiveInPlaceholders[Idx]->replaceAllUsesWith(Op); | ||
|
|
||
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 you update the comment for
SmallVector<VPSingleDefRecipe *> ExpressionRecipes(line 2973) that this vector may contain duplicates?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.
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 is not done?
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.
Sorry, I did add it but must not have staged it.