-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[LV] Use ExtractLane(LastActiveLane, V) live outs when tail-folding. #149042
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
+1,626
−584
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
b7b23a9
[LV] Use ExtractLane(LastActiveLane, V) live outs when tail-folding.
fhahn 9985387
Merge remote-tracking branch 'origin/main' into lv-tf-external-users
fhahn 6b0c9d2
!fixup add LastActiveLane opcode, lowered via FirstActiveLane.
fhahn 2a45f94
Merge remote-tracking branch 'origin/main' into lv-tf-external-users
fhahn 7d1ab3c
!fixup verify LastActiveLane.
fhahn 8ca0426
Merge remote-tracking branch 'origin/main' into lv-tf-external-users
fhahn 96c827b
[VPlan] Mark VPlan argument in isHeaderMask as const (NFC).
fhahn 72f3796
!fixup clean up verifier changes
fhahn f3fdac7
Merge remote-tracking branch 'origin/main' into lv-tf-external-users
fhahn af82079
!fixup add note about operands needing to be prefix-masks.
fhahn cc7e913
!fixup fix formatting
fhahn 2d3012f
Merge remote-tracking branch 'origin/main' into lv-tf-external-users
fhahn 4fa7442
!fixup tighten verifier check for broadcast of EVL
fhahn ae81e08
Merge remote-tracking branch 'origin/main' into lv-tf-external-users
fhahn f10fa37
!fixup add additional pattern matching helpers.
fhahn 2758f96
Merge remote-tracking branch 'origin/main' into lv-tf-external-users
fhahn 73b3197
Merge remote-tracking branch 'origin/main' into lv-tf-external-users
fhahn d8e77ca
!fixup address comments, thanks
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
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.
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.
I think we have to be careful about this transformation because it's making the assumption that there are no holes in the mask. For example, you'd presumably get something unexpected if you transform LastActiveLane(11100111000) -> FirstActiveLane(00011000111).
It might require adding documentation to the opcodes saying that holes are not permitted when using the LastActiveLane opcode. I don't know if there is a way to add an assert here that the mask doesn't contain holes? Presumably it should only be used when the mask has originally come get.active.lane.mask?
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.
Yep, for now the operands will always be header masks (or EVL based masks), so we can verify that (added to VPlanVerifier). Updated and extended the comment for the opcode, thanks
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.
So I've added a similar opcode (ExtractLastActive) as part of https://github.com/llvm/llvm-project/pull/158088/files#diff-dabd8bf9956285f5ddab712af4e0494647a106234f07283b0bd9f8b4d9b887ca , and that does expect that there might be holes.
Should we keep the opcodes separate, or express one in terms of the other and enforce the 'contiguousness' of active lanes directly for your code?
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.
Having the LastActiveLane check separate has the potential advantage that it could expose more CSE/simplification opportuntiies. The restriction for prefix-masks at the moment is only there due to how it gets lowered using FirstActiveLane.
I think we can just remove the restriction once we have a use-case for non-prefix-masks and add dedicated lowering that supports them.
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 really can't think of when we'd end up using LastActiveLane with anything but the header mask.
Would it be simpler to just make LastActiveLane a nullary op, and just fetch the header mask with
findHeaderMaskwhen converting to a concrete recipe? That way we don't have to worry about the non-contiguous invariant.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.
Unfortunately I don't think that's possible at the moment, as at the point when we call covnertToConcreteRecipes, the region may have been removed (or if the LastActiveLane would be the only user of the header mask it may have been removed), so I've left it as-is for now
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'm also realising now that we need to model the use of the header mask to prevent removeDeadRecipes from stripping it if e.g. it's EVL tail folded and all other uses of it are removed.