[mlir][linalg] BufferizeToAllocationOp: fix side effects
#72986
Merged
Conversation
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
`bufferize_to_allocation` does not bufferize/replace ops if `bufferize_destination_only` is set.
matthias-springer
requested review from
MaheshRavishankar and
nicolasvasilache
|
@llvm/pr-subscribers-mlir-linalg Author: Matthias Springer (matthias-springer) Changes
Fixes #72931. Full diff: https://github.com/llvm/llvm-project/pull/72986.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp b/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
index de4965f937162ea..ef5d88d46dd28a0 100644
--- a/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
+++ b/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
@@ -289,7 +289,13 @@ DiagnosedSilenceableFailure transform::BufferizeToAllocationOp::apply(
void transform::BufferizeToAllocationOp::getEffects(
SmallVectorImpl<MemoryEffects::EffectInstance> &effects) {
- consumesHandle(getTarget(), effects);
+ if (getBufferizeDestinationOnly()) {
+ // The destination is replaced with a newly allocated buffer, but the op
+ // itself remains in place.
+ onlyReadsHandle(getTarget(), effects);
+ } else {
+ consumesHandle(getTarget(), effects);
+ }
producesHandle(getAllocatedBuffer(), effects);
producesHandle(getNewOps(), effects);
modifiesPayload(effects);
|
|
@llvm/pr-subscribers-mlir Author: Matthias Springer (matthias-springer) Changes
Fixes #72931. Full diff: https://github.com/llvm/llvm-project/pull/72986.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp b/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
index de4965f937162ea..ef5d88d46dd28a0 100644
--- a/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
+++ b/mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
@@ -289,7 +289,13 @@ DiagnosedSilenceableFailure transform::BufferizeToAllocationOp::apply(
void transform::BufferizeToAllocationOp::getEffects(
SmallVectorImpl<MemoryEffects::EffectInstance> &effects) {
- consumesHandle(getTarget(), effects);
+ if (getBufferizeDestinationOnly()) {
+ // The destination is replaced with a newly allocated buffer, but the op
+ // itself remains in place.
+ onlyReadsHandle(getTarget(), effects);
+ } else {
+ consumesHandle(getTarget(), effects);
+ }
producesHandle(getAllocatedBuffer(), effects);
producesHandle(getNewOps(), effects);
modifiesPayload(effects);
|
ftynse
approved these changes
Nov 22, 2023
matthias-springer
added a commit
to matthias-springer/llvm-project
that referenced
this pull request
Nov 28, 2023
When running with "expensive checks", the transform dialect interpreter maintains a payload `Operation *` -> `OperationName` cache. This cache is used to detect invalid API usage such as missing/incorrect handle consumption/production side effects and/or payload IR modifications that bypass the rewriter. There was a bug in the check that can cause issues such as llvm#72931. (llvm#72986 was just a workaround and did not really fix the underlying issue.) - Payload ops mapped to newly produced handles are now added to the cache. This is in addition to adding/checking all mapped payload ops at the beginning of each transform op, for extra safety. - Remove consumed ops (and their children) before applying the transform op. This used to happen after applying the transform op, which is incorrect in cases such as: (1) transform op replaces a consumed payload op with another op, (2) the new op reuses the same memory pointer and (3) the new op is added to a newly produced handle. In such a case the previous implementation removed the newly created op from the cache. - No assumptions can be made about whether an op should be in the cache or not. The code previously reported an error when an op was not found in the cache. E.g., this is problematic in cases such as: (1) the transform op consumes the handle mapped to a payload op A and (2) the implementation of the payload op removes/replaces a nested op with A, which is mapped to another handle. This triggers a listener notification, which removes the nested op from the cache. However, because consumed ops (and their children) are removed from the cache before applying the transform op, the nested op will not be in cache and making such an assumption would be incorrect.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
bufferize_to_allocationdoes not bufferize/replace targeted ops ifbufferize_destination_onlyis set.Fixes #72931.