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.
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
[mlir][bufferization]
MaterializeInDestinationOp
: Support memref destinations #68074[mlir][bufferization]
MaterializeInDestinationOp
: Support memref destinations #68074Changes from all commits
037663f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 are deeper safety concerns here right ?
We should make it more explicit, here and below, that using this op improperly will result in undefined behavior.
I think the current wording here and above is potentially error prone in that the user may expect the analysis to be conservative or produce warnings and/or errors.
In reality this is a very strict directive that is easy to misuse.
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.
Added this sentence here and at
ToTensorOp
:Ops that have incorrect usage of
restrictmay bufferize incorrectly.
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.
is this true ?
If one uses tensor.insert_slice, the analysis will insert copies if required, whereas
materialize_in_destination
is prescriptive IIUC.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.
You are right, that is the desired behavior. It was actually not like that for tensor destinations.
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.
another note maybe: is there any correctness guarantee when e.g. using this op as the last op in a function ?
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.
As long as
restrict
is not used incorrectly, the IR is guaranteed to bufferize correctly. If the computation cannot materialize in the specified tensor due to a RaW conflict or a read-only tensor, the IR fails to bufferize. (Added test cases.)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.
same here, can we be more explicit about user's responsibility / UB vs what the analysis will catch ?
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.
Added this sentence:
Ops that have incorrect usage of restrict may bufferize incorrectly.
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.
Not sure this actually reads better.
Using only the type of the tensor of the memref is fully unambiguous, no?
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 because the result is optional. In case of a
memref
, there is no result (likelinalg.generic
). TableGen helpers such asAllTypesMatch
etc. do not work with that. I could write the printer/parser in C++ though, then we could stay with the original syntax.