[mlir][transform] Fix new interpreter and library preloading passes. #69190
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
|
@llvm/pr-subscribers-mlir Author: Ingo Müller (ingomueller-net) ChangesThis PR fixes the two recently added passes from #68661, which were non-functional and untested. In particular:
Full diff: https://github.com/llvm/llvm-project/pull/69190.diff 5 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Transform/Transforms/Passes.td b/mlir/include/mlir/Dialect/Transform/Transforms/Passes.td
index c900fee76b814d3..d65b6786118b32f 100644
--- a/mlir/include/mlir/Dialect/Transform/Transforms/Passes.td
+++ b/mlir/include/mlir/Dialect/Transform/Transforms/Passes.td
@@ -49,10 +49,11 @@ def PreloadLibraryPass : Pass<"transform-preload-library"> {
transform interpreter passes. The preloading occurs into the Transform
dialect and thus provides very limited functionality that does not scale.
- Warning: Only a single such pass should exist for a given MLIR context.
- This is a temporary solution until a resource-based solution is available.
- TODO: investigate using a resource blob if some ownership mode allows it.
+ Warning: This is a temporary solution until a resource-based solution is
+ available.
}];
+ // TODO: investigate using a resource blob if some ownership mode allows it.
+ let dependentDialects = ["transform::TransformDialect"];
let options = [
ListOption<"transformLibraryPaths", "transform-library-paths", "std::string",
"Optional paths to files with modules that should be merged into the "
@@ -67,6 +68,7 @@ def InterpreterPass : Pass<"transform-interpreter"> {
sequence transformation specified by the provided name (defaults to
`__transform_main`).
}];
+ let dependentDialects = ["transform::TransformDialect"];
let options = [
Option<"entryPoint", "entry-point", "std::string",
/*default=*/[{"__transform_main"}],
diff --git a/mlir/lib/Dialect/Transform/Transforms/InterpreterPass.cpp b/mlir/lib/Dialect/Transform/Transforms/InterpreterPass.cpp
index f473d5aa728c519..3ec51d88729a0e7 100644
--- a/mlir/lib/Dialect/Transform/Transforms/InterpreterPass.cpp
+++ b/mlir/lib/Dialect/Transform/Transforms/InterpreterPass.cpp
@@ -25,13 +25,10 @@ class InterpreterPass
public:
using Base::Base;
- LogicalResult initialize(MLIRContext *context) override {
- // TODO: investigate using a resource blob if some ownership mode allows it.
- transformModule = transform::detail::getPreloadedTransformModule(context);
- return success();
- }
-
void runOnOperation() override {
+ MLIRContext *context = &getContext();
+ ModuleOp transformModule =
+ transform::detail::getPreloadedTransformModule(context);
if (failed(transform::applyTransformNamedSequence(
getOperation(), transformModule,
options.enableExpensiveChecks(true), entryPoint)))
@@ -41,11 +38,5 @@ class InterpreterPass
private:
/// Transform interpreter options.
transform::TransformOptions options;
-
- /// The separate transform module to be used for transformations, shared
- /// across multiple instances of the pass if it is applied in parallel to
- /// avoid potentially expensive cloning. MUST NOT be modified after the pass
- /// has been initialized.
- ModuleOp transformModule;
};
} // namespace
diff --git a/mlir/test/Dialect/Transform/interpreter-entry-point.mlir b/mlir/test/Dialect/Transform/interpreter-entry-point.mlir
new file mode 100644
index 000000000000000..ccd9bef3d506ddf
--- /dev/null
+++ b/mlir/test/Dialect/Transform/interpreter-entry-point.mlir
@@ -0,0 +1,17 @@
+// RUN: mlir-opt %s -transform-interpreter=entry-point=entry_point \
+// RUN: -split-input-file -verify-diagnostics
+
+module attributes { transform.with_named_sequence } {
+ transform.named_sequence @entry_point(!transform.any_op {transform.readonly}) {
+ ^bb0(%arg0: !transform.any_op):
+ // expected-remark @below {{applying transformation}}
+ transform.test_transform_op
+ transform.yield
+ }
+
+ transform.named_sequence @__transform_main(!transform.any_op {transform.readonly}) {
+ ^bb0(%arg0: !transform.any_op):
+ transform.test_transform_op // Note: does not yield remark.
+ transform.yield
+ }
+}
diff --git a/mlir/test/Dialect/Transform/interpreter.mlir b/mlir/test/Dialect/Transform/interpreter.mlir
new file mode 100644
index 000000000000000..bb41420bef4d63c
--- /dev/null
+++ b/mlir/test/Dialect/Transform/interpreter.mlir
@@ -0,0 +1,17 @@
+// RUN: mlir-opt %s -transform-interpreter \
+// RUN: -split-input-file -verify-diagnostics
+
+module attributes { transform.with_named_sequence } {
+ transform.named_sequence @__transform_main(!transform.any_op {transform.readonly}) {
+ ^bb0(%arg0: !transform.any_op):
+ // expected-remark @below {{applying transformation}}
+ transform.test_transform_op
+ transform.yield
+ }
+
+ transform.named_sequence @entry_point(!transform.any_op {transform.readonly}) {
+ ^bb0(%arg0: !transform.any_op):
+ transform.test_transform_op // Note: does not yield remark.
+ transform.yield
+ }
+}
diff --git a/mlir/test/Dialect/Transform/preload-library.mlir b/mlir/test/Dialect/Transform/preload-library.mlir
new file mode 100644
index 000000000000000..55ae51bcd63b4b4
--- /dev/null
+++ b/mlir/test/Dialect/Transform/preload-library.mlir
@@ -0,0 +1,19 @@
+// RUN: mlir-opt %s \
+// RUN: -transform-preload-library=transform-library-paths=%p%{fs-sep}test-interpreter-library \
+// RUN: -transform-interpreter=entry-point=private_helper \
+// RUN: -split-input-file -verify-diagnostics
+
+// expected-remark @below {{message}}
+module {}
+
+// -----
+
+// Note: no remark here since local entry point takes precedence.
+module attributes { transform.with_named_sequence } {
+ transform.named_sequence @private_helper(!transform.any_op {transform.readonly}) {
+ ^bb0(%arg0: !transform.any_op):
+ // expected-remark @below {{applying transformation}}
+ transform.test_transform_op
+ transform.yield
+ }
+}
\ No newline at end of file
|
This PR fixes the two recently added passes from llvm#68661, which were non-functional and untested. In particular: * The passes did not declare their dependent dialects, so they could not run at all in the most simple cases. * The mechanism of loading the library module in the initialization of the intepreter pass is broken by design (but, fortunately, also not necessary). This is because the initialization of all passes happens before the execution of any other pass, so the "preload library" pass has not run yet at the time the interpreter pass gets initialized. Instead, the library is now loaded every time the interpreter pass is run. This should not be exceedingly expensive, since it only consists of looking up the library in the dialect. Also, this removes the library module from the pass state, making it possible in the future to preload libraries in several passes. * The PR adds tests for the two passes, which were completely untested previously.
ingomueller-net
force-pushed
the
fix-transform-preload-interpreter
branch
from
5e3381d
to
05a5db5
Compare
ingomueller-net
requested review from
nicolasvasilache,
ftynse and
matthias-springer
ftynse
reviewed
Oct 16, 2023
Independently of this PR, I'm concerned by this, I'll comment on the original PR! |
|
Thanks for the review. Nits addressed in latest commit.
Sure:
|
ingomueller-net
added a commit
to ingomueller-net/llvm-project
that referenced
this pull request
Oct 17, 2023
A recent commit (llvm#69190) broke the bazel builds. Turns out that Bazel uses symlinks for providing the test files, which the path expansion of the module loading mechanism did not handle correctly. This PR fixes that. It also reorganizes the tests better: It puts all `.mlir` files that are included by some other test into a common `include` folder. This greatly simplifies the definition of the dependencies between the different `.mlir` files in Bazel's `BUILD` file. The commit also adds a comment to all included files why these aren't tested themselves direclty and uses the `%{fs-sep}` expansion for paths more consistently. Finally, it uncomments all but one of the tests excluded in Bazel because they seem to run now. (The remaining one includes a file that it itself a test, so it would have to live *in* and *outside* of the `include` folder.)
This was referenced Oct 17, 2023
I don't think it was approved though, was it? |
I see, thanks. What really escaped my attention here is that a pass cannot load a dialect because it run in multithreaded mode, so it has to declare as dependent any dialect it needed to load and have it loaded by the pass manager. This is different from creating entities from the dialect, which is impossible without the dialect being loaded in the first place. Not sure if this is documented sufficiently, somehow it wasn't obvious to me.
Indeed, it wasn't. I haven't requested changes as I was rather asking for clarification. Approved after-the-fact, but generally better to wait for approval in the future. |
Thanks for the explanation! Those subtleties had escape my attention completely, so it's great to have them brought to my attention.
So sorry!! This wasn't on purpose -- I had assumed/thought that it was approved, which it wasn't. Will be more careful next time! |
ingomueller-net
added a commit
that referenced
this pull request
Oct 19, 2023
…s. (#69329) A recent commit (#69190) broke the bazel builds. Turns out that Bazel uses symlinks for providing the test files, which the path expansion of the module loading mechanism did not handle correctly. This PR fixes that. It also reorganizes the tests better: It puts all `.mlir` files that are included by some other test into a common `include` folder. This greatly simplifies the definition of the dependencies between the different `.mlir` files in Bazel's `BUILD` file. The commit also adds a comment to all included files why these aren't tested themselves direclty and uses the `%{fs-sep}` expansion for paths more consistently. Finally, it uncomments all but one of the tests excluded in Bazel because they seem to run now. (The remaining one includes a file that it itself a test, so it would have to live *in* and *outside* of the `include` folder.)
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 PR fixes the two recently added passes from #68661, which were non-functional and untested. In particular: