[mlir] Link libraries that aren't included in libMLIR to libMLIR #123477
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
mgorny
requested review from
vchuravy,
nikic,
stephenneuendorffer and
River707
|
@llvm/pr-subscribers-mlir-sparse @llvm/pr-subscribers-mlir Author: Michał Górny (mgorny) ChangesUse I have only covered the libraries that are used by Flang. If you wish, I can extend this approach to all non-libMLIR libraries in MLIR, making MLIR itself also link to the dylib consistently. Full diff: https://github.com/llvm/llvm-project/pull/123477.diff 3 Files Affected:
diff --git a/mlir/test/lib/Analysis/CMakeLists.txt b/mlir/test/lib/Analysis/CMakeLists.txt
index 7c6b31ae8b73e5..8135d22d1f7baa 100644
--- a/mlir/test/lib/Analysis/CMakeLists.txt
+++ b/mlir/test/lib/Analysis/CMakeLists.txt
@@ -21,12 +21,18 @@ add_mlir_library(MLIRTestAnalysis
EXCLUDE_FROM_LIBMLIR
LINK_LIBS PUBLIC
+ MLIRTestDialect
+ )
+
+# Since this library is excluded from libMLIR, link it to the dylib
+# to ensure that it can be used in flang without implicitly pulling in
+# static libraries.
+mlir_target_link_libraries(MLIRTestAnalysis PUBLIC
MLIRAffineDialect
MLIRAnalysis
MLIRFunctionInterfaces
MLIRMemRefDialect
MLIRPass
- MLIRTestDialect
)
target_include_directories(MLIRTestAnalysis
diff --git a/mlir/test/lib/Dialect/Test/CMakeLists.txt b/mlir/test/lib/Dialect/Test/CMakeLists.txt
index 967101242e26b5..aebf4724be34e9 100644
--- a/mlir/test/lib/Dialect/Test/CMakeLists.txt
+++ b/mlir/test/lib/Dialect/Test/CMakeLists.txt
@@ -68,8 +68,12 @@ add_mlir_library(MLIRTestDialect
MLIRTestOpsIncGen
MLIRTestOpsSyntaxIncGen
MLIRTestOpsShardGen
+ )
- LINK_LIBS PUBLIC
+# Since this library is excluded from libMLIR, link it to the dylib
+# to ensure that it can be used in flang without implicitly pulling in
+# static libraries.
+mlir_target_link_libraries(MLIRTestDialect PUBLIC
MLIRControlFlowInterfaces
MLIRDataLayoutInterfaces
MLIRDerivedAttributeOpInterface
diff --git a/mlir/test/lib/IR/CMakeLists.txt b/mlir/test/lib/IR/CMakeLists.txt
index 01297ad0a11482..09e370007a424e 100644
--- a/mlir/test/lib/IR/CMakeLists.txt
+++ b/mlir/test/lib/IR/CMakeLists.txt
@@ -27,8 +27,12 @@ add_mlir_library(MLIRTestIR
TestVisitorsGeneric.cpp
EXCLUDE_FROM_LIBMLIR
+ )
- LINK_LIBS PUBLIC
+# Since this library is excluded from libMLIR, link it to the dylib
+# to ensure that it can be used in flang without implicitly pulling in
+# static libraries.
+mlir_target_link_libraries(MLIRTestIR PUBLIC
MLIRPass
MLIRBytecodeReader
MLIRBytecodeWriter
|
nikic
reviewed
Jan 18, 2025
There was a problem hiding this comment.
This is a good idea, thanks.
I have only covered the libraries that are used by Flang. If you wish, I can extend this approach to all non-libMLIR libraries in MLIR, making MLIR itself also link to the dylib consistently.
Yes, we should do this for everything.
Use `mlir_target_link_libraries()` to link dependencies of libraries that are not included in libMLIR, to ensure that they link to the dylib when they are used in Flang. Otherwise, they implicitly pull in all their static dependencies, effectively causing Flang binaries to simultaneously link to the dylib and to static libraries, which is never a good idea. I have only covered the libraries that are used by Flang. If you wish, I can extend this approach to all non-libMLIR libraries in MLIR, making MLIR itself also link to the dylib consistently.
Hopefully I didn't mess anything up. The tests build and pass for me (except for these that failed already) both with MLIR_LINK_MLIR_DYLIB and without.
mgorny
force-pushed
the
mlir-test-lib-dylib-link
branch
from
bc0a0ef to
b391ba2
Compare
mgorny
requested review from
ftynse,
makslevental,
stellaraccident,
grypp,
aartbik,
PeimingLiu,
yinying-lisa-li,
matthias-springer,
antiagainst,
kuhar,
banach-space,
dcaballe and
nicolasvasilache
as code owners
|
Rebased and updated for all the libs. |
llvmbot
added
mlir:core
llvmbot
added
mlir:vectorops
mlir:sparse
nikic
approved these changes
Jan 20, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/89/builds/14755 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/138/builds/9138 Here is the relevant piece of the build log for the reference |
|
Looks like I broke |
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.
Use
mlir_target_link_libraries()to link dependencies of libraries that are not included in libMLIR, to ensure that they link to the dylib when they are used in Flang. Otherwise, they implicitly pull in all their static dependencies, effectively causing Flang binaries to simultaneously link to the dylib and to static libraries, which is never a good idea.I have only covered the libraries that are used by Flang. If you wish, I can extend this approach to all non-libMLIR libraries in MLIR, making MLIR itself also link to the dylib consistently.