-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[MLIR][Vector] Implement TransferOpReduceRank as MaskableOpRewritePattern #92426
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
3 commits
Select commit
Hold shift + click to select a range
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
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.
How is this test different to
@permutation_with_mask_xfer_read_scalable
? Both seem to check forvector.transfer_read
frommemref
into a scalable vector?Uh oh!
There was an error while loading. Please reload this page.
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.
There is a transposition in the permutation map of
permutation_with_mask_xfer_read_scalable
. It would hence also triggerTransferReadPermutationLowering
which IIRC would fail without #91987 if I masked it withvector.mask
. Changes were not completely independent, this one is a bit more unit test-likeThere 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.
Cool, right now that's very unclear, unless you happen to know what to look for. Such distinction should be captured either:
(preferably both). That's currently not the case, so lets fix that. I'm suggesting the following plan-of-action:
no_transpose
to the test functions names that you added.I can take it from there.
I have two more high-level points. A bit tangential to this PR - more like TODOs for myself 😅
What test should be included in vector-transfer-permutation-lowering.mlir?
Btw, based on the file name and the patterns being tested here:
transform.apply_patterns.vector.transfer_permutation_patterns
,there should be no non-permutation examples in this file, right? To make things even more confusing, these patterns are also tested in:
😱 :) I'm on a mission to tidy this up a bit (while adding tests for scalable vectors).
Note on populateVectorTransferPermutationMapLoweringPatterns
Separately, it's bit confusing that
TransferOpReduceRank
is added topopulateVectorTransferPermutationMapLoweringPatterns
:llvm-project/mlir/lib/Dialect/Vector/Transforms/LowerVectorTransfer.cpp
Lines 410 to 416 in 77db8b0
I think that it would be easier to "categorise" tests if the granularity was lower. Another PR though. I can handle 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.
Makes sense to me, it deserves refactoring and standardization. I could have done better on commenting and naming.
Alternatively, separating tests with a big bold comment by pattern they are actually testing from the set could make sense.
I think this quite goes against the idea of having a permutation specific file. as you mention there :
But I think we should rather emphasize on the fact it is not a permutation lowering but a lowering based on the permutation map . Hence the
no_transpose
answer works.I would not mind about
transform-vector.mlir
as it is a full pipeline test. The goal is to show and test a complete lowering which makes sense. A bit like the e-2-e examples you added for SME.For vector-transfer-to-vector-load-store.mlir , I think this file deserves improvements too. (such as the repetition of same transform sequences. ) The concept is to combine it with lower_transfer to test the complete lowering of xfer ops. it is not very unit test like but it shows how to properly lower xfer ops which is a benefit. Mixed feelings. I think there should not be
transfer_permutation_patterns
specific tests here, but I find sensible to test this combination of patterns. We should maybe ask @ftynse .Indeed, but after all it is a lowering based on the permutation map of a TransferRead. Hence, I think than rather removing
TransferOpReduceRank
, the ideal solution is to renameTransferOpReduceRank
to make clear it matches patterns in the permutation map or renamingpopulateVectorTransferPermutationMapLoweringPatterns
to reflect it is not mandatory a permutation. This set of patterns is useful as a whole for the community as a lowering pass.I would also add something. As @hanhanW mentioned in #93664 (review) we could get rid of
/// ``` and standardize it.
in Doxygen descriptions for improved readability.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.
It is also helpful when I parse the comment in the code directly. I'd appreciate if someone can help make it consistent. :)
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 syntax is mostly for markdown to me, so I don't know why people started adding such style to comments.
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.
That's spot-on and something that I missed. In fact, this is documented here:
llvm-project/mlir/include/mlir/Dialect/Vector/Transforms/LoweringPatterns.h
Lines 175 to 228 in 77db8b0
With this in mind, please ignore my comments under this heading:
In fact, sounds like vector-transfer-permutation-lowering.mlir is the right file for all patterns under
populateVectorTransferPermutationMapLoweringPatterns
.Going back to the problem at hand ... This
and this:
First step in this direction: #95529. Let me know if that makes sense and I will prepare more tests.
Not your fault, it's hard to navigate this ATM (this is also why it takes me ages to review things). Clearly our test coverage is not great either. In general, Vector needs some TLC 😅 I really appreciate you helping us here 🙏🏻
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.
Next one: #96033