[mlir][bufferization] Simplify helper potentiallyAliasesMemref
#78690
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
|
@llvm/pr-subscribers-mlir-bufferization Author: Matthias Springer (matthias-springer) ChangesThis commit simplifies a helper function in the ownership-based buffer deallocation pass. Full diff: https://github.com/llvm/llvm-project/pull/78690.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Bufferization/Transforms/BufferDeallocationSimplification.cpp b/mlir/lib/Dialect/Bufferization/Transforms/BufferDeallocationSimplification.cpp
index 4d1e21c2e406cd..e30779868b4753 100644
--- a/mlir/lib/Dialect/Bufferization/Transforms/BufferDeallocationSimplification.cpp
+++ b/mlir/lib/Dialect/Bufferization/Transforms/BufferDeallocationSimplification.cpp
@@ -77,18 +77,12 @@ static bool distinctAllocAndBlockArgument(Value v1, Value v2) {
return areDistinct(v1Base, v2Base) || areDistinct(v2Base, v1Base);
}
-/// Checks if `memref` may or must alias a MemRef in `otherList`. It is often a
-/// requirement of optimization patterns that there cannot be any aliasing
-/// memref in order to perform the desired simplification. The `allowSelfAlias`
-/// argument indicates whether `memref` may be present in `otherList` which
-/// makes this helper function applicable to situations where we already know
-/// that `memref` is in the list but also when we don't want it in the list.
+/// Checks if `memref` may potentially alias a MemRef in `otherList`. It is
+/// often a requirement of optimization patterns that there cannot be any
+/// aliasing memref in order to perform the desired simplification.
static bool potentiallyAliasesMemref(AliasAnalysis &analysis,
- ValueRange otherList, Value memref,
- bool allowSelfAlias) {
+ ValueRange otherList, Value memref) {
for (auto other : otherList) {
- if (allowSelfAlias && other == memref)
- continue;
if (distinctAllocAndBlockArgument(other, memref))
continue;
if (!analysis.alias(other, memref).isNo())
@@ -243,7 +237,7 @@ struct RemoveRetainedMemrefsGuaranteedToNotAlias
for (auto retainedMemref : deallocOp.getRetained()) {
if (potentiallyAliasesMemref(aliasAnalysis, deallocOp.getMemrefs(),
- retainedMemref, false)) {
+ retainedMemref)) {
newRetainedMemrefs.push_back(retainedMemref);
replacements.push_back({});
continue;
@@ -314,11 +308,13 @@ struct SplitDeallocWhenNotAliasingAnyOther
SmallVector<Value> remainingMemrefs, remainingConditions;
SmallVector<SmallVector<Value>> updatedConditions;
- for (auto [memref, cond] :
- llvm::zip(deallocOp.getMemrefs(), deallocOp.getConditions())) {
+ for (int64_t i = 0, e = deallocOp.getMemrefs().size(); i < e; ++i) {
+ Value memref = deallocOp.getMemrefs()[i];
+ Value cond = deallocOp.getConditions()[i];
+ SmallVector<Value> otherMemrefs(deallocOp.getMemrefs());
+ otherMemrefs.erase(otherMemrefs.begin() + i);
// Check if `memref` can split off into a separate bufferization.dealloc.
- if (potentiallyAliasesMemref(aliasAnalysis, deallocOp.getMemrefs(),
- memref, true)) {
+ if (potentiallyAliasesMemref(aliasAnalysis, otherMemrefs, memref)) {
// `memref` alias with other memrefs, do not split off.
remainingMemrefs.push_back(memref);
remainingConditions.push_back(cond);
|
|
@llvm/pr-subscribers-mlir Author: Matthias Springer (matthias-springer) ChangesThis commit simplifies a helper function in the ownership-based buffer deallocation pass. Full diff: https://github.com/llvm/llvm-project/pull/78690.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Bufferization/Transforms/BufferDeallocationSimplification.cpp b/mlir/lib/Dialect/Bufferization/Transforms/BufferDeallocationSimplification.cpp
index 4d1e21c2e406cd3..e30779868b47539 100644
--- a/mlir/lib/Dialect/Bufferization/Transforms/BufferDeallocationSimplification.cpp
+++ b/mlir/lib/Dialect/Bufferization/Transforms/BufferDeallocationSimplification.cpp
@@ -77,18 +77,12 @@ static bool distinctAllocAndBlockArgument(Value v1, Value v2) {
return areDistinct(v1Base, v2Base) || areDistinct(v2Base, v1Base);
}
-/// Checks if `memref` may or must alias a MemRef in `otherList`. It is often a
-/// requirement of optimization patterns that there cannot be any aliasing
-/// memref in order to perform the desired simplification. The `allowSelfAlias`
-/// argument indicates whether `memref` may be present in `otherList` which
-/// makes this helper function applicable to situations where we already know
-/// that `memref` is in the list but also when we don't want it in the list.
+/// Checks if `memref` may potentially alias a MemRef in `otherList`. It is
+/// often a requirement of optimization patterns that there cannot be any
+/// aliasing memref in order to perform the desired simplification.
static bool potentiallyAliasesMemref(AliasAnalysis &analysis,
- ValueRange otherList, Value memref,
- bool allowSelfAlias) {
+ ValueRange otherList, Value memref) {
for (auto other : otherList) {
- if (allowSelfAlias && other == memref)
- continue;
if (distinctAllocAndBlockArgument(other, memref))
continue;
if (!analysis.alias(other, memref).isNo())
@@ -243,7 +237,7 @@ struct RemoveRetainedMemrefsGuaranteedToNotAlias
for (auto retainedMemref : deallocOp.getRetained()) {
if (potentiallyAliasesMemref(aliasAnalysis, deallocOp.getMemrefs(),
- retainedMemref, false)) {
+ retainedMemref)) {
newRetainedMemrefs.push_back(retainedMemref);
replacements.push_back({});
continue;
@@ -314,11 +308,13 @@ struct SplitDeallocWhenNotAliasingAnyOther
SmallVector<Value> remainingMemrefs, remainingConditions;
SmallVector<SmallVector<Value>> updatedConditions;
- for (auto [memref, cond] :
- llvm::zip(deallocOp.getMemrefs(), deallocOp.getConditions())) {
+ for (int64_t i = 0, e = deallocOp.getMemrefs().size(); i < e; ++i) {
+ Value memref = deallocOp.getMemrefs()[i];
+ Value cond = deallocOp.getConditions()[i];
+ SmallVector<Value> otherMemrefs(deallocOp.getMemrefs());
+ otherMemrefs.erase(otherMemrefs.begin() + i);
// Check if `memref` can split off into a separate bufferization.dealloc.
- if (potentiallyAliasesMemref(aliasAnalysis, deallocOp.getMemrefs(),
- memref, true)) {
+ if (potentiallyAliasesMemref(aliasAnalysis, otherMemrefs, memref)) {
// `memref` alias with other memrefs, do not split off.
remainingMemrefs.push_back(memref);
remainingConditions.push_back(cond);
|
maerhart
approved these changes
Jan 19, 2024
Comment on lines
+314
to
+315
| SmallVector<Value> otherMemrefs(deallocOp.getMemrefs()); | ||
| otherMemrefs.erase(otherMemrefs.begin() + i); |
There was a problem hiding this comment.
This probably even fixes a bug when the same memref is contained in deallocOp.getMemrefs() multiple times. The old implementation would allow splitting which would lead to a double-free if I understand correctly without actually testing it or looking into it in detail. Do we have a test-case for that?
There was a problem hiding this comment.
I tried to write a test for that. We have another pattern that removes duplicate memrefs (DeallocRemoveDuplicateDeallocMemrefs). I think whether the bug is triggered or not depends on the order in which the patterns are applied. I was not able to trigger the double dealloc in practice. (I'll add the test case back anyway, just to be sure.)
There was a problem hiding this comment.
Makes sense. It's a bit annoying that MLIR doesn't have the utility to easily test individual patterns :/
This commit simplifies a helper function in the ownership-based buffer deallocation pass.
matthias-springer
force-pushed
the
simplify_potentially_aliases_memref
branch
from
59a0a93 to
6dcf21e
Compare
matthias-springer
changed the title
potentiallyAliasesMemrefpotentiallyAliasesMemref
aartbik
reviewed
Jan 22, 2024
mlir/lib/Dialect/Bufferization/Transforms/BufferDeallocationSimplification.cpp
Show resolved
Hide resolved
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.
This commit simplifies a helper function in the ownership-based buffer deallocation pass. Fixes a potential double-free (depending on the scheduling of patterns).