-
Notifications
You must be signed in to change notification settings - Fork 13.5k
[VPlan] Don't collect live-ins in collectUsersInExitBlocks. (NFC) #123819
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
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
d0e00e8
[VPlan] Don't collect live-ins in collectUsersInExitBlocks. (NFC)
fhahn 3d30a34
!fixup move early exit exit value handling to VPlanTransform.
fhahn 8994cdf
Merge remote-tracking branch 'origin/main' into vplan-handle-live-in-…
fhahn 021e4bd
!fixup introduce and use VPIRInstruction::extractLastLaneOfOperand.
fhahn 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
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.
Can you hold off on this patch for a bit until I've tested the interaction with #120567 please? I'm worried this essentially means that there is no sensible way to get hold of the predecessor.
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 definitely the patch tries to help simplify #120567, so we should only land this if it helps.
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.
OK, I think I now see what you're doing here. I thought this was just a temporary solution to get at least one test working, but it sounds like you want to take the approach of duplicating the code in addUsersInExitBlocks for the latch exit and hide the live-outs from the
ExitUsersToFix
list in LoopVectorize.cpp. This will work, but my concern here is that the code inhandleUncountableEarlyExit
may get left behind when fixes/improvements are made for loops without early exits.ExitUsersToFix
is also passed in toaddExitUsersForFirstOrderRecurrences
and I can see in future that list might be used for something else. Although it will probably be a while before support is added for first order recurrences, it might mean this code has to be rewritten at some point.I understand what you mean in your comment on #120567 about this avoiding the problem of ordering when calling
collectUsersInExitBlocks
, followed byhandleUncountableEarlyExit
. I think if we go with this solution I'd prefer to see bothaddUsersInEarlyExitBlock
andhandleUncountableEarlyExit
call a common function to manage the live-out, i.e. pull this code out:That way anyone modifying this code is forced to take early exit loops into consideration too. There is already an assert in
addExitUsersForFirstOrderRecurrences
that there is no early exit so at least that is defended.Apologies if I am being a little paranoid here, but until the early exit work is enabled by default I just want to avoid something getting broken along the way. :)
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.
Sounds good, I added
VPIRInstruction::extractLastLaneOfOperand
, is that along the lines you had in mind?Now that all live-outs are handled in VPlan I am planning to look into simplifying and unifying the place where we handle the exit users. If/once there's support for reductions/first-order recurrences for early exit loops, I think they should ideally also handled directly in
handleUncountableEarlyExit
, but that's for another day :)