[mlir][bufferization] Add deallocation option to remove existing dealloc operations, add option to specify the kind of alloc operations to consider #67556
Conversation
This file contains 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
llvmbot
added
mlir:core
…ction boundary ABI Inserting clones requires a lot of assumptions to hold on the input IR, e.g., all writes to a buffer need to dominate all reads. This is not guaranteed by one-shot bufferization and isn't easy to verify, thus it could quickly lead to incorrect results that are hard to debug. This commit changes the mechanism of how an ownership indicator is materialized when there is not already a unique ownership present. Additionally, we don't create copies of returned memrefs anymore when we don't have ownership. Instead, we insert assert operations to make sure we have ownership at runtime, or otherwise report to the user that correctness could not be guaranteed.
…loc operations, add option to specify the kind of alloc operations to consider
maerhart
force-pushed
the
merhart_dealloc_mixed_support
branch
from
c2d180a
to
590efce
Compare
matthias-springer
approved these changes
Sep 28, 2023
| /// ownership will be set to 'false', i.e., it will be leaked. This is useful | ||
| /// to support deallocation of multiple different kinds of allocation ops. | ||
| DetectionFn isRelevantAllocOp = [](Operation *op) { | ||
| return isa<memref::MemRefDialect, bufferization::BufferizationDialect>( |
There was a problem hiding this comment.
Why is the bufferization dialect in here? Can we just check for memref.alloc/memref.alloca in here?
There was a problem hiding this comment.
BufferizationDialect is here because of bufferization.clone which has an allocation side-effect as well.
| @@ -106,6 +111,48 @@ struct DeallocationOptions { | |||
| /// to, an error will already be emitted at compile time. This cannot be | |||
| /// changed with this option. | |||
| bool verifyFunctionBoundaryABI = true; | |||
|
|
|||
| /// Given an allocation side-effect on the passed operation, determine whether | |||
There was a problem hiding this comment.
Given an operation with an allocation side effect
| op->getDialect()); | ||
| }; | ||
|
|
||
| /// Given a free side-effect on the passed operation, determine whether this |
There was a problem hiding this comment.
Given an operation with a free side effect
| PassOptions::Option<bool> removeExistingDeallocations{ | ||
| *this, "remove-existing-deallocations", | ||
| llvm::cl::desc("Removes all pre-existing memref.dealloc operations and " | ||
| "insert all deallocations according to the buffer " |
There was a problem hiding this comment.
I think just Removes pre-existing memref.dealloc operations should be enough.
| @@ -233,6 +233,11 @@ def OwnershipBasedBufferDeallocation : Pass< | |||
| "If it can be determined statically that the ABI is not adhered " | |||
| "to, an error will already be emitted at compile time. This cannot " | |||
| "be changed with this option.">, | |||
| Option<"removeExistingDeallocations", "remove-existing-deallocations", | |||
| "bool", /*default=*/"false", | |||
| "Remove already existing MemRef deallocation operations and let the " | |||
| } | ||
|
|
||
| // Alloc operations from other dialects are expected to have matching | ||
| // deallocation operations inserted by another pass. |
There was a problem hiding this comment.
inserted by a separate run of this pass
|
|
||
| OpBuilder builder = OpBuilder::atBlockBegin(block); | ||
| for (auto res : llvm::make_filter_range(op->getResults(), isMemref)) { | ||
| auto allocEffect = op.getEffectOnValue<MemoryEffects::Allocate>(res); | ||
| if (allocEffect.has_value()) { | ||
| // Assuming that an alloc effect is interpreted as MUST and not MAY. | ||
| state.resetOwnerships(res, block); |
|
|
||
| WalkResult result = getOperation()->walk([&](FunctionOpInterface funcOp) { | ||
| // Deallocate the `memref.alloc` operations. | ||
| bufferization::DeallocationOptions options; |
There was a problem hiding this comment.
Add comment that by default the deallocation options are configured to look for memref.alloc/memref.dealloc only.
| } | ||
| return ValueRange{}; | ||
| } | ||
| return failure(); |
There was a problem hiding this comment.
Can be llvm_unreachable("expected gpu.dealloc op")? Then you could also make the gpuDealloc a cast<DeallocOp> and you won't need the "if" check.
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.
Depends on #66626
Only review top commit.