[mlir][Transforms] Tighten replaceUsesOfBlockArgument
#155227
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
|
@llvm/pr-subscribers-mlir Author: Matthias Springer (matthias-springer) ChangesImprove the documentation of This commit is in preparation of adding full support for Full diff: https://github.com/llvm/llvm-project/pull/155227.diff 2 Files Affected:
diff --git a/mlir/include/mlir/Transforms/DialectConversion.h b/mlir/include/mlir/Transforms/DialectConversion.h
index 536b23f5c33c1..f23a70601fc0a 100644
--- a/mlir/include/mlir/Transforms/DialectConversion.h
+++ b/mlir/include/mlir/Transforms/DialectConversion.h
@@ -782,6 +782,12 @@ class ConversionPatternRewriter final : public PatternRewriter {
/// Replace all the uses of the block argument `from` with `to`. This
/// function supports both 1:1 and 1:N replacements.
+ ///
+ /// Note: If `allowPatternRollback` is set to "true", this function replaces
+ /// all current and future uses of the block argument. This same block
+ /// block argument must not be replaced multiple times. Uses are not replaced
+ /// immediately but in a delayed fashion. Patterns may still see the original
+ /// uses when inspecting IR.
void replaceUsesOfBlockArgument(BlockArgument from, ValueRange to);
/// Return the converted value of 'key' with a type defined by the type
diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 7494ca9ec3784..e3248204d6694 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -1132,6 +1132,11 @@ struct ConversionPatternRewriterImpl : public RewriterBase::Listener {
IRRewriter notifyingRewriter;
#ifndef NDEBUG
+ /// A set of replaced block arguments. This set is for debugging purposes
+ /// only and it is maintained only if `allowPatternRollback` is set to
+ /// "true".
+ DenseSet<BlockArgument> replacedArgs;
+
/// A set of operations that have pending updates. This tracking isn't
/// strictly necessary, and is thus only active during debug builds for extra
/// verification.
@@ -1892,6 +1897,19 @@ void ConversionPatternRewriterImpl::replaceUsesOfBlockArgument(
return;
}
+#ifndef NDEBUG
+ // Make sure that a block argument is not replaced multiple times. In
+ // rollback mode, `replaceUsesOfBlockArgument` replaces not only all current
+ // uses of the given block argument, but also all future uses that may be
+ // introduced by future pattern applications. Therefore, it does not make
+ // sense to call `replaceUsesOfBlockArgument` multiple times with the same
+ // block argument. Doing so would overwrite the mapping and mess with the
+ // internal state of the dialect conversion driver.
+ assert(!replacedArgs.contains(from) &&
+ "attempting to replace a block argument that was already replaced");
+ replacedArgs.insert(from);
+#endif // NDEBUG
+
appendRewrite<ReplaceBlockArgRewrite>(from.getOwner(), from, converter);
mapping.map(from, to);
}
|
|
@llvm/pr-subscribers-mlir-core Author: Matthias Springer (matthias-springer) ChangesImprove the documentation of This commit is in preparation of adding full support for Full diff: https://github.com/llvm/llvm-project/pull/155227.diff 2 Files Affected:
diff --git a/mlir/include/mlir/Transforms/DialectConversion.h b/mlir/include/mlir/Transforms/DialectConversion.h
index 536b23f5c33c1..f23a70601fc0a 100644
--- a/mlir/include/mlir/Transforms/DialectConversion.h
+++ b/mlir/include/mlir/Transforms/DialectConversion.h
@@ -782,6 +782,12 @@ class ConversionPatternRewriter final : public PatternRewriter {
/// Replace all the uses of the block argument `from` with `to`. This
/// function supports both 1:1 and 1:N replacements.
+ ///
+ /// Note: If `allowPatternRollback` is set to "true", this function replaces
+ /// all current and future uses of the block argument. This same block
+ /// block argument must not be replaced multiple times. Uses are not replaced
+ /// immediately but in a delayed fashion. Patterns may still see the original
+ /// uses when inspecting IR.
void replaceUsesOfBlockArgument(BlockArgument from, ValueRange to);
/// Return the converted value of 'key' with a type defined by the type
diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 7494ca9ec3784..e3248204d6694 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -1132,6 +1132,11 @@ struct ConversionPatternRewriterImpl : public RewriterBase::Listener {
IRRewriter notifyingRewriter;
#ifndef NDEBUG
+ /// A set of replaced block arguments. This set is for debugging purposes
+ /// only and it is maintained only if `allowPatternRollback` is set to
+ /// "true".
+ DenseSet<BlockArgument> replacedArgs;
+
/// A set of operations that have pending updates. This tracking isn't
/// strictly necessary, and is thus only active during debug builds for extra
/// verification.
@@ -1892,6 +1897,19 @@ void ConversionPatternRewriterImpl::replaceUsesOfBlockArgument(
return;
}
+#ifndef NDEBUG
+ // Make sure that a block argument is not replaced multiple times. In
+ // rollback mode, `replaceUsesOfBlockArgument` replaces not only all current
+ // uses of the given block argument, but also all future uses that may be
+ // introduced by future pattern applications. Therefore, it does not make
+ // sense to call `replaceUsesOfBlockArgument` multiple times with the same
+ // block argument. Doing so would overwrite the mapping and mess with the
+ // internal state of the dialect conversion driver.
+ assert(!replacedArgs.contains(from) &&
+ "attempting to replace a block argument that was already replaced");
+ replacedArgs.insert(from);
+#endif // NDEBUG
+
appendRewrite<ReplaceBlockArgRewrite>(from.getOwner(), from, converter);
mapping.map(from, to);
}
|
|
Note for LLVM integration: If you are seeing failures due to this change, look for |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/17422 Here is the relevant piece of the build log for the reference |
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.
Improve the documentation of
replaceUsesOfBlockArgumentto clarify its semantics in rollback mode. Add an assertion to make sure that the same block argument is not replaced multiple times. That's an API violation and messes with the internal state of the conversion driver.This commit is in preparation of adding full support for
RewriterBase::replaceAllUsesWith.