[mlir][transform] Support symlinks in module loading. Reorganize tests. #69329
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
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.)
|
@llvm/pr-subscribers-mlir Author: Ingo Müller (ingomueller-net) ChangesA 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 Full diff: https://github.com/llvm/llvm-project/pull/69329.diff 15 Files Affected:
diff --git a/mlir/lib/Dialect/Transform/Transforms/TransformInterpreterUtils.cpp b/mlir/lib/Dialect/Transform/Transforms/TransformInterpreterUtils.cpp
index 41feffffaf97b3f..e6d692072267c1f 100644
--- a/mlir/lib/Dialect/Transform/Transforms/TransformInterpreterUtils.cpp
+++ b/mlir/lib/Dialect/Transform/Transforms/TransformInterpreterUtils.cpp
@@ -61,7 +61,8 @@ LogicalResult transform::detail::expandPathsToMLIRFiles(
it != itEnd && !ec; it.increment(ec)) {
const std::string &fileName = it->path();
- if (it->type() != llvm::sys::fs::file_type::regular_file) {
+ if (it->type() != llvm::sys::fs::file_type::regular_file &&
+ it->type() != llvm::sys::fs::file_type::symlink_file) {
LLVM_DEBUG(DBGS() << " Skipping non-regular file '" << fileName
<< "'\n");
continue;
diff --git a/mlir/test/Dialect/Transform/Library/lower-to-llvm.mlir b/mlir/test/Dialect/Transform/include/Library/lower-to-llvm.mlir
similarity index 96%
rename from mlir/test/Dialect/Transform/Library/lower-to-llvm.mlir
rename to mlir/test/Dialect/Transform/include/Library/lower-to-llvm.mlir
index 0ba50bd2362b34d..afd1c89dd2b52f6 100644
--- a/mlir/test/Dialect/Transform/Library/lower-to-llvm.mlir
+++ b/mlir/test/Dialect/Transform/include/Library/lower-to-llvm.mlir
@@ -1,4 +1,5 @@
// RUN: mlir-opt %s
+// No need to check anything else than parsing here, this is being used by another test as data.
/// Schedule to lower to LLVM.
module @lower_module_to_llvm attributes { transform.with_named_sequence } {
diff --git a/mlir/test/Dialect/Transform/test-interpreter-external-concurrent-source.mlir b/mlir/test/Dialect/Transform/include/test-interpreter-external-concurrent-source.mlir
similarity index 100%
rename from mlir/test/Dialect/Transform/test-interpreter-external-concurrent-source.mlir
rename to mlir/test/Dialect/Transform/include/test-interpreter-external-concurrent-source.mlir
diff --git a/mlir/test/Dialect/Transform/test-interpreter-external-source.mlir b/mlir/test/Dialect/Transform/include/test-interpreter-external-source.mlir
similarity index 100%
rename from mlir/test/Dialect/Transform/test-interpreter-external-source.mlir
rename to mlir/test/Dialect/Transform/include/test-interpreter-external-source.mlir
diff --git a/mlir/test/Dialect/Transform/test-interpreter-external-symbol-def-invalid.mlir b/mlir/test/Dialect/Transform/include/test-interpreter-external-symbol-def-invalid.mlir
similarity index 100%
rename from mlir/test/Dialect/Transform/test-interpreter-external-symbol-def-invalid.mlir
rename to mlir/test/Dialect/Transform/include/test-interpreter-external-symbol-def-invalid.mlir
diff --git a/mlir/test/Dialect/Transform/test-interpreter-library/definitions-self-contained.mlir b/mlir/test/Dialect/Transform/include/test-interpreter-library/definitions-self-contained.mlir
similarity index 96%
rename from mlir/test/Dialect/Transform/test-interpreter-library/definitions-self-contained.mlir
rename to mlir/test/Dialect/Transform/include/test-interpreter-library/definitions-self-contained.mlir
index 66f0f1f62683b7e..58a8f76c5791a27 100644
--- a/mlir/test/Dialect/Transform/test-interpreter-library/definitions-self-contained.mlir
+++ b/mlir/test/Dialect/Transform/include/test-interpreter-library/definitions-self-contained.mlir
@@ -1,4 +1,5 @@
// RUN: mlir-opt %s
+// No need to check anything else than parsing here, this is being used by another test as data.
module attributes {transform.with_named_sequence} {
transform.named_sequence private @private_helper(%arg0: !transform.any_op {transform.readonly}) {
diff --git a/mlir/test/Dialect/Transform/test-interpreter-library/definitions-with-unresolved.mlir b/mlir/test/Dialect/Transform/include/test-interpreter-library/definitions-with-unresolved.mlir
similarity index 78%
rename from mlir/test/Dialect/Transform/test-interpreter-library/definitions-with-unresolved.mlir
rename to mlir/test/Dialect/Transform/include/test-interpreter-library/definitions-with-unresolved.mlir
index b3d076f4698495f..a3b315952b30970 100644
--- a/mlir/test/Dialect/Transform/test-interpreter-library/definitions-with-unresolved.mlir
+++ b/mlir/test/Dialect/Transform/include/test-interpreter-library/definitions-with-unresolved.mlir
@@ -1,4 +1,5 @@
// RUN: mlir-opt %s
+// No need to check anything else than parsing here, this is being used by another test as data.
module attributes {transform.with_named_sequence} {
transform.named_sequence @print_message(%arg0: !transform.any_op {transform.readonly})
diff --git a/mlir/test/Dialect/Transform/preload-library.mlir b/mlir/test/Dialect/Transform/preload-library.mlir
index 61d22252dc61dfd..9beefa44d673d9f 100644
--- a/mlir/test/Dialect/Transform/preload-library.mlir
+++ b/mlir/test/Dialect/Transform/preload-library.mlir
@@ -1,5 +1,5 @@
// RUN: mlir-opt %s \
-// RUN: -transform-preload-library=transform-library-paths=%p%{fs-sep}test-interpreter-library \
+// RUN: -transform-preload-library=transform-library-paths=%p%{fs-sep}include%{fs-sep}test-interpreter-library \
// RUN: -transform-interpreter=entry-point=private_helper \
// RUN: -split-input-file -verify-diagnostics
diff --git a/mlir/test/Dialect/Transform/test-interpreter-external-concurrent.mlir b/mlir/test/Dialect/Transform/test-interpreter-external-concurrent.mlir
index 46a1a130d9bcb37..59c2b672a6e6b10 100644
--- a/mlir/test/Dialect/Transform/test-interpreter-external-concurrent.mlir
+++ b/mlir/test/Dialect/Transform/test-interpreter-external-concurrent.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s --pass-pipeline="builtin.module(func.func(test-transform-dialect-interpreter{transform-file-name=%p/test-interpreter-external-concurrent-source.mlir}))" \
+// RUN: mlir-opt %s --pass-pipeline="builtin.module(func.func(test-transform-dialect-interpreter{transform-file-name=%p%{fs-sep}include%{fs-sep}test-interpreter-external-concurrent-source.mlir}))" \
// RUN: --verify-diagnostics
// Exercising the pass on multiple functions of different lengths that may be
diff --git a/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl-and-schedule.mlir b/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl-and-schedule.mlir
index 2c4812bf32b0f03..9e50ec1efac946c 100644
--- a/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl-and-schedule.mlir
+++ b/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl-and-schedule.mlir
@@ -1,7 +1,7 @@
-// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-file-name=%p/test-interpreter-external-symbol-decl.mlir transform-library-paths=%p/test-interpreter-library/definitions-self-contained.mlir})" \
+// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-file-name=%p%{fs-sep}test-interpreter-external-symbol-decl.mlir transform-library-paths=%p%{fs-sep}include%{fs-sep}test-interpreter-library/definitions-self-contained.mlir})" \
// RUN: --verify-diagnostics
-// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-file-name=%p/test-interpreter-external-symbol-decl.mlir transform-library-paths=%p/test-interpreter-library/definitions-self-contained.mlir}, test-transform-dialect-interpreter{transform-file-name=%p/test-interpreter-external-symbol-decl.mlir transform-library-paths=%p/test-interpreter-library/definitions-self-contained.mlir})" \
+// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-file-name=%p%{fs-sep}test-interpreter-external-symbol-decl.mlir transform-library-paths=%p%{fs-sep}include%{fs-sep}test-interpreter-library/definitions-self-contained.mlir}, test-transform-dialect-interpreter{transform-file-name=%p%{fs-sep}test-interpreter-external-symbol-decl.mlir transform-library-paths=%p%{fs-sep}include%{fs-sep}test-interpreter-library/definitions-self-contained.mlir})" \
// RUN: --verify-diagnostics
// The external transform script has a declaration to the named sequence @foo,
diff --git a/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl-dir.mlir b/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl-dir.mlir
index 8b8254976e9aeec..3681b913dc5b974 100644
--- a/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl-dir.mlir
+++ b/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl-dir.mlir
@@ -1,10 +1,10 @@
-// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-library-paths=%p%{fs-sep}test-interpreter-library})" \
+// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-library-paths=%p%{fs-sep}include%{fs-sep}test-interpreter-library})" \
// RUN: --verify-diagnostics --split-input-file | FileCheck %s
-// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-library-paths=%p%{fs-sep}test-interpreter-library/definitions-self-contained.mlir,%p%{fs-sep}test-interpreter-library/definitions-with-unresolved.mlir})" \
+// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-library-paths=%p%{fs-sep}include%{fs-sep}test-interpreter-library/definitions-self-contained.mlir,%p%{fs-sep}include%{fs-sep}test-interpreter-library/definitions-with-unresolved.mlir})" \
// RUN: --verify-diagnostics --split-input-file | FileCheck %s
-// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-library-paths=%p%{fs-sep}test-interpreter-library}, test-transform-dialect-interpreter)" \
+// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-library-paths=%p%{fs-sep}include%{fs-sep}test-interpreter-library}, test-transform-dialect-interpreter)" \
// RUN: --verify-diagnostics --split-input-file | FileCheck %s
// The definition of the @foo named sequence is provided in another file. It
diff --git a/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl-invalid.mlir b/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl-invalid.mlir
index c1bd071dc138d56..060dab334ed4388 100644
--- a/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl-invalid.mlir
+++ b/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl-invalid.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-library-paths=%p/test-interpreter-external-symbol-def-invalid.mlir}, test-transform-dialect-interpreter)" \
+// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-library-paths=%p%{fs-sep}include%{fs-sep}test-interpreter-external-symbol-def-invalid.mlir}, test-transform-dialect-interpreter)" \
// RUN: --verify-diagnostics --split-input-file
// The definition of the @print_message named sequence is provided in another file. It
diff --git a/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl.mlir b/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl.mlir
index 339e62072cd5510..8a35e981bd48b76 100644
--- a/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl.mlir
+++ b/mlir/test/Dialect/Transform/test-interpreter-external-symbol-decl.mlir
@@ -1,7 +1,7 @@
-// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-library-paths=%p/test-interpreter-library/definitions-self-contained.mlir})" \
+// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-library-paths=%p%{fs-sep}include%{fs-sep}test-interpreter-library/definitions-self-contained.mlir})" \
// RUN: --verify-diagnostics --split-input-file | FileCheck %s
-// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-library-paths=%p/test-interpreter-library/definitions-self-contained.mlir}, test-transform-dialect-interpreter)" \
+// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-library-paths=%p%{fs-sep}include%{fs-sep}test-interpreter-library/definitions-self-contained.mlir}, test-transform-dialect-interpreter)" \
// RUN: --verify-diagnostics --split-input-file | FileCheck %s
// The definition of the @print_message named sequence is provided in another
diff --git a/mlir/test/Dialect/Transform/test-interpreter-external.mlir b/mlir/test/Dialect/Transform/test-interpreter-external.mlir
index 5ac6b66c817afef..ba8e0c6870dbf8a 100644
--- a/mlir/test/Dialect/Transform/test-interpreter-external.mlir
+++ b/mlir/test/Dialect/Transform/test-interpreter-external.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-file-name=%p/test-interpreter-external-source.mlir})" \
+// RUN: mlir-opt %s --pass-pipeline="builtin.module(test-transform-dialect-interpreter{transform-file-name=%p%{fs-sep}include%{fs-sep}test-interpreter-external-source.mlir})" \
// RUN: --verify-diagnostics
// The schedule in the separate file emits remarks at the payload root.
diff --git a/utils/bazel/llvm-project-overlay/mlir/test/Dialect/BUILD.bazel b/utils/bazel/llvm-project-overlay/mlir/test/Dialect/BUILD.bazel
index e5b877a48d5e848..1fd6885db8bca93 100644
--- a/utils/bazel/llvm-project-overlay/mlir/test/Dialect/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/mlir/test/Dialect/BUILD.bazel
@@ -18,11 +18,7 @@ package(default_visibility = ["//visibility:public"])
] + glob([
"IRDL/*.irdl.mlir",
"LLVM/*-symbol-def.mlir",
- "Transform/*-source.mlir",
- "Transform/*-symbol-def.mlir",
- "Transform/*-symbol-decl-and-schedule.mlir",
- "Transform/Library/*.mlir",
- "Transform/test-interpreter-library/*.mlir",
+ "Transform/include/**/*.mlir",
]),
)
for src in glob(
@@ -30,15 +26,8 @@ package(default_visibility = ["//visibility:public"])
exclude = [
"IRDL/*.irdl.mlir",
"LLVM/*-symbol-def.mlir",
- "Transform/*-source.mlir",
- "Transform/*-symbol-def.mlir",
"Transform/*-symbol-decl-and-schedule.mlir",
- "Transform/*-symbol-decl-dir.mlir",
- "Transform/*-symbol-decl-invalid.mlir",
- "Transform/Library/*.mlir",
- "Transform/preload-library.mlir",
- "Transform/test-interpreter-library/*.mlir",
- "Transform/test-repro-dump.mlir",
+ "Transform/include/**/*.mlir",
],
)
]
|
ftynse
approved these changes
Oct 18, 2023
ingomueller-net
deleted the
fix-transform-module-merging-symlinks-and-bazel-tests
branch
ingomueller-net
added a commit
to ingomueller-net/llvm-project
that referenced
this pull request
Oct 25, 2023
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.
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
.mlirfiles that are included by some other test into a commonincludefolder. This greatly simplifies the definition of the dependencies between the different.mlirfiles in Bazel'sBUILDfile. 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 theincludefolder.)