-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[VPlan] Expand VPBlendRecipes to select instructions. NFC #133993
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
lukel97
merged 13 commits into
llvm:main
from
lukel97:loop-vectorize/simplifiy-blend-select
Jul 23, 2025
Merged
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
0b07987
[VPlan] Simplify VPBlendRecipes to select instructions
lukel97 bb56e67
Update VPlan debug output tests
lukel97 e0a2fbd
Mark in planContainsAdditionalSimplifications
lukel97 2081698
Merge branch 'main' of github.com:llvm/llvm-project into loop-vectori…
lukel97 76bee12
Merge branch 'main' of github.com:llvm/llvm-project into loop-vectori…
lukel97 4911b73
Use getUnderlyingInstr
lukel97 78c3a08
Revert "Use getUnderlyingInstr"
lukel97 a3c0340
Precisely track blend selects to see if they have been simplified, ad…
lukel97 6774abe
Merge branch 'main' of github.com:llvm/llvm-project into loop-vectori…
lukel97 b39d6f5
Don't simplify, just expand in convertToConcreteRecipes
lukel97 d95e7cd
Remove redundant setUnderlyingValue, adjust comment
lukel97 6de02ac
Move VPBlendRecipe::execute into header
lukel97 2db0c0a
Merge branch 'main' of github.com:llvm/llvm-project into loop-vectori…
lukel97 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back 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.
Why is this needed? This effectively disables cost checking for any plans with blends? If the blend gets simplified away, the regular checks should already catch this?
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 issue is that when only part of the blend gets simplified away, e.g. if we have
Which was costed as
(getNumIncomingValues() - 1) * TTI.getCmpSelInstrCostWith this patch this then gets expanded to
And then simplified to just
So the cost is now just
1 * TTI.getCmpSelInstrCost.Because one select still remains and it has an underlying instruction it doesn't trigger the generic regular checks.
We could just not set the underlying instruction when simplifying the blend to selects, although that has the same effect of disabling cost checking for any plans with blends.
I've tried making the check more precise in a3c0340 so now we should only disable the cost checking if a select has actually been removed, and I've added a test (this was harder than I expected because the ordering of successors matters, which can differ between .ll and .bc files! #147038)
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.
Argh I see, so we create multiple selects with the same underlying instruction and this is causing the issue.
As lowering blends to selects becomes mandatory, it would probably be better to do it in convertToConcreteRecipes, at least to start with, removing the cost-model issue?
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.
We could do it in convertToConcreteRecipes but then we don't get any of the benefits of the simplifications that occur on selects, both in
VPlanTransforms::simplifyRecipesand inoptimizeMaskToEVL.Although I'm happy to at least start in convertToConcreteRecipes if you think it'll be ok to move the transform to earlier in a follow up patch?
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 that would probably be good as a first step, to start with removing VPBlendRecipe::execute.
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've moved this to convertToConcreteRecipes in b39d6f5, this should be more or less NFC now