-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[mlir] Fix MLIR dylib CMake config when LLVM_BUILD_LLVM_DYLIB is OFF #155488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Currently, MLIR fails to build when LLVM_BUILD_LLVM_DYLIB is OFF and MLIR_LINK_MLIR_DYLIB is ON. The reason for this is that the MLIR shared library target is only defined when LLVM_BUILD_LLVM_DYLIB is set to ON, but the `mlir_target_link_libraries` CMake function will attempt to link its given target against the MLIR shared library target whenever the MLIR_LINK_MLIR_DYLIB option is ON. Furthermore, there is a second issue where the MLIR target is referenced by `mlir_target_link_libraries` calls _before_ it is defined by `tools/mlir-shlib/CMakeLists.txt`. To address the above problems, this commit introduces a new MLIR_BUILD_MLIR_DYLIB CMake option that is used to determine whether the MLIR shared libary target should be built. Similar to LLVM_BUILD_LLVM_DYLIB, the corresponding MLIR option is automatically enabled whenever MLIR_LINK_MLIR_DYLIB is ON. The `mlir_target_link_libraries` now links against an interface target MLIRDylib, which "foward declares" the MLIR shared library target, as the latter cannot be defined until after all the individual libraries are.
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
@llvm/pr-subscribers-mlir-core Author: Bryan Tan (Technius) ChangesCurrently, MLIR fails to build when LLVM_BUILD_LLVM_DYLIB is OFF and MLIR_LINK_MLIR_DYLIB is ON. The reason for this is that the MLIR shared library target is only defined when LLVM_BUILD_LLVM_DYLIB is set to ON, but the Furthermore, there is a second issue where the MLIR target is referenced by To address the above problems, this commit introduces a new MLIR_BUILD_MLIR_DYLIB CMake option that is used to determine whether the MLIR shared libary target should be built. Similar to LLVM_BUILD_LLVM_DYLIB, the corresponding MLIR option is automatically enabled whenever MLIR_LINK_MLIR_DYLIB is ON. The Full diff: https://github.com/llvm/llvm-project/pull/155488.diff 4 Files Affected:
diff --git a/mlir/CMakeLists.txt b/mlir/CMakeLists.txt
index f58a4c6f506ec..371740292323a 100644
--- a/mlir/CMakeLists.txt
+++ b/mlir/CMakeLists.txt
@@ -172,8 +172,11 @@ set(MLIR_INSTALL_AGGREGATE_OBJECTS 1 CACHE BOOL
set(MLIR_BUILD_MLIR_C_DYLIB 0 CACHE BOOL "Builds libMLIR-C shared library.")
-set(MLIR_LINK_MLIR_DYLIB ${LLVM_LINK_LLVM_DYLIB} CACHE BOOL
- "Link tools against libMLIR.so")
+set(MLIR_BUILD_MLIR_DYLIB 0 CACHE BOOL "Builds the libMLIR shared library")
+set(MLIR_LINK_MLIR_DYLIB ${LLVM_LINK_LLVM_DYLIB} CACHE BOOL "Link tools against libMLIR.so")
+if (MLIR_LINK_MLIR_DYLIB)
+ set(MLIR_BUILD_MLIR_DYLIB 1)
+endif()
configure_file(
${MLIR_MAIN_INCLUDE_DIR}/mlir/Config/mlir-config.h.cmake
@@ -235,6 +238,15 @@ set(MLIR_PDLL_TABLEGEN_TARGET "${MLIR_PDLL_TABLEGEN_TARGET}" CACHE INTERNAL "")
set(MLIR_SRC_SHARDER_TABLEGEN_EXE "${MLIR_SRC_SHARDER_TABLEGEN_EXE}" CACHE INTERNAL "")
set(MLIR_SRC_SHARDER_TABLEGEN_TARGET "${MLIR_SRC_SHARDER_TABLEGEN_TARGET}" CACHE INTERNAL "")
+# Add MLIR dylib target here, as calls to mlir_target_link_libraries (used by
+# individual libs below) assume that such a target may exist,
+# but we cannot define the actual dylib until after all individual libs
+# are defined.
+if (MLIR_LINK_MLIR_DYLIB)
+ add_library(MLIRDylib INTERFACE)
+ add_mlir_library_install(MLIRDylib)
+endif()
+
add_subdirectory(include/mlir)
add_subdirectory(lib)
# C API needs all dialects for registration, but should be built before tests.
diff --git a/mlir/cmake/modules/AddMLIR.cmake b/mlir/cmake/modules/AddMLIR.cmake
index 38839679ef8b1..9545eab265db3 100644
--- a/mlir/cmake/modules/AddMLIR.cmake
+++ b/mlir/cmake/modules/AddMLIR.cmake
@@ -354,11 +354,11 @@ function(add_mlir_library name)
# Yes, because the target "obj.${name}" is referenced.
set(NEEDS_OBJECT_LIB ON)
endif ()
- if(LLVM_BUILD_LLVM_DYLIB AND NOT ARG_EXCLUDE_FROM_LIBMLIR AND NOT XCODE)
+ if(MLIR_BUILD_MLIR_DYLIB AND NOT ARG_EXCLUDE_FROM_LIBMLIR AND NOT XCODE)
# Yes, because in addition to the shared library, the object files are
# needed for linking into libMLIR.so (see mlir/tools/mlir-shlib/CMakeLists.txt).
# For XCode, -force_load is used instead.
- # Windows is not supported (LLVM_BUILD_LLVM_DYLIB=ON will cause an error).
+ # Windows is not supported (MLIR_BUILD_MLIR_DYLIB=ON will cause an error).
set(NEEDS_OBJECT_LIB ON)
set_property(GLOBAL APPEND PROPERTY MLIR_STATIC_LIBS ${name})
set_property(GLOBAL APPEND PROPERTY MLIR_LLVM_LINK_COMPONENTS ${ARG_LINK_COMPONENTS})
@@ -745,7 +745,7 @@ function(mlir_target_link_libraries target type)
endif()
if (MLIR_LINK_MLIR_DYLIB)
- target_link_libraries(${target} ${type} MLIR)
+ target_link_libraries(${target} ${type} MLIRDylib)
else()
target_link_libraries(${target} ${type} ${ARGN})
endif()
diff --git a/mlir/lib/ExecutionEngine/CMakeLists.txt b/mlir/lib/ExecutionEngine/CMakeLists.txt
index fdeb4dacf9278..85cbb1ef3bc4e 100644
--- a/mlir/lib/ExecutionEngine/CMakeLists.txt
+++ b/mlir/lib/ExecutionEngine/CMakeLists.txt
@@ -103,7 +103,7 @@ mlir_target_link_libraries(MLIRExecutionEngine PUBLIC
MLIRTargetLLVMIRExport
)
-if(LLVM_BUILD_LLVM_DYLIB AND NOT (WIN32 OR MINGW OR CYGWIN)) # Does not build on windows currently, see #106859
+if(MLIR_BUILD_MLIR_DYLIB AND NOT (WIN32 OR MINGW OR CYGWIN)) # Does not build on windows currently, see #106859
# Build a shared library for the execution engine. Some downstream projects
# use this library to build their own CPU runners while preserving dynamic
# linkage.
diff --git a/mlir/tools/mlir-shlib/CMakeLists.txt b/mlir/tools/mlir-shlib/CMakeLists.txt
index a33c70c5807be..166bab744de36 100644
--- a/mlir/tools/mlir-shlib/CMakeLists.txt
+++ b/mlir/tools/mlir-shlib/CMakeLists.txt
@@ -30,7 +30,7 @@ if(MLIR_LINK_MLIR_DYLIB)
set(INSTALL_WITH_TOOLCHAIN INSTALL_WITH_TOOLCHAIN)
endif()
-if(LLVM_BUILD_LLVM_DYLIB)
+if(MLIR_BUILD_MLIR_DYLIB)
add_mlir_library(
MLIR
SHARED
@@ -45,6 +45,7 @@ if(LLVM_BUILD_LLVM_DYLIB)
${mlir_llvm_link_components}
)
target_link_libraries(MLIR PRIVATE ${LLVM_PTHREAD_LIB})
+ target_link_libraries(MLIRDylib INTERFACE MLIR)
endif()
#message("Libraries included in libMLIR.so: ${mlir_libs}")
|
@llvm/pr-subscribers-mlir-execution-engine Author: Bryan Tan (Technius) ChangesCurrently, MLIR fails to build when LLVM_BUILD_LLVM_DYLIB is OFF and MLIR_LINK_MLIR_DYLIB is ON. The reason for this is that the MLIR shared library target is only defined when LLVM_BUILD_LLVM_DYLIB is set to ON, but the Furthermore, there is a second issue where the MLIR target is referenced by To address the above problems, this commit introduces a new MLIR_BUILD_MLIR_DYLIB CMake option that is used to determine whether the MLIR shared libary target should be built. Similar to LLVM_BUILD_LLVM_DYLIB, the corresponding MLIR option is automatically enabled whenever MLIR_LINK_MLIR_DYLIB is ON. The Full diff: https://github.com/llvm/llvm-project/pull/155488.diff 4 Files Affected:
diff --git a/mlir/CMakeLists.txt b/mlir/CMakeLists.txt
index f58a4c6f506ec..371740292323a 100644
--- a/mlir/CMakeLists.txt
+++ b/mlir/CMakeLists.txt
@@ -172,8 +172,11 @@ set(MLIR_INSTALL_AGGREGATE_OBJECTS 1 CACHE BOOL
set(MLIR_BUILD_MLIR_C_DYLIB 0 CACHE BOOL "Builds libMLIR-C shared library.")
-set(MLIR_LINK_MLIR_DYLIB ${LLVM_LINK_LLVM_DYLIB} CACHE BOOL
- "Link tools against libMLIR.so")
+set(MLIR_BUILD_MLIR_DYLIB 0 CACHE BOOL "Builds the libMLIR shared library")
+set(MLIR_LINK_MLIR_DYLIB ${LLVM_LINK_LLVM_DYLIB} CACHE BOOL "Link tools against libMLIR.so")
+if (MLIR_LINK_MLIR_DYLIB)
+ set(MLIR_BUILD_MLIR_DYLIB 1)
+endif()
configure_file(
${MLIR_MAIN_INCLUDE_DIR}/mlir/Config/mlir-config.h.cmake
@@ -235,6 +238,15 @@ set(MLIR_PDLL_TABLEGEN_TARGET "${MLIR_PDLL_TABLEGEN_TARGET}" CACHE INTERNAL "")
set(MLIR_SRC_SHARDER_TABLEGEN_EXE "${MLIR_SRC_SHARDER_TABLEGEN_EXE}" CACHE INTERNAL "")
set(MLIR_SRC_SHARDER_TABLEGEN_TARGET "${MLIR_SRC_SHARDER_TABLEGEN_TARGET}" CACHE INTERNAL "")
+# Add MLIR dylib target here, as calls to mlir_target_link_libraries (used by
+# individual libs below) assume that such a target may exist,
+# but we cannot define the actual dylib until after all individual libs
+# are defined.
+if (MLIR_LINK_MLIR_DYLIB)
+ add_library(MLIRDylib INTERFACE)
+ add_mlir_library_install(MLIRDylib)
+endif()
+
add_subdirectory(include/mlir)
add_subdirectory(lib)
# C API needs all dialects for registration, but should be built before tests.
diff --git a/mlir/cmake/modules/AddMLIR.cmake b/mlir/cmake/modules/AddMLIR.cmake
index 38839679ef8b1..9545eab265db3 100644
--- a/mlir/cmake/modules/AddMLIR.cmake
+++ b/mlir/cmake/modules/AddMLIR.cmake
@@ -354,11 +354,11 @@ function(add_mlir_library name)
# Yes, because the target "obj.${name}" is referenced.
set(NEEDS_OBJECT_LIB ON)
endif ()
- if(LLVM_BUILD_LLVM_DYLIB AND NOT ARG_EXCLUDE_FROM_LIBMLIR AND NOT XCODE)
+ if(MLIR_BUILD_MLIR_DYLIB AND NOT ARG_EXCLUDE_FROM_LIBMLIR AND NOT XCODE)
# Yes, because in addition to the shared library, the object files are
# needed for linking into libMLIR.so (see mlir/tools/mlir-shlib/CMakeLists.txt).
# For XCode, -force_load is used instead.
- # Windows is not supported (LLVM_BUILD_LLVM_DYLIB=ON will cause an error).
+ # Windows is not supported (MLIR_BUILD_MLIR_DYLIB=ON will cause an error).
set(NEEDS_OBJECT_LIB ON)
set_property(GLOBAL APPEND PROPERTY MLIR_STATIC_LIBS ${name})
set_property(GLOBAL APPEND PROPERTY MLIR_LLVM_LINK_COMPONENTS ${ARG_LINK_COMPONENTS})
@@ -745,7 +745,7 @@ function(mlir_target_link_libraries target type)
endif()
if (MLIR_LINK_MLIR_DYLIB)
- target_link_libraries(${target} ${type} MLIR)
+ target_link_libraries(${target} ${type} MLIRDylib)
else()
target_link_libraries(${target} ${type} ${ARGN})
endif()
diff --git a/mlir/lib/ExecutionEngine/CMakeLists.txt b/mlir/lib/ExecutionEngine/CMakeLists.txt
index fdeb4dacf9278..85cbb1ef3bc4e 100644
--- a/mlir/lib/ExecutionEngine/CMakeLists.txt
+++ b/mlir/lib/ExecutionEngine/CMakeLists.txt
@@ -103,7 +103,7 @@ mlir_target_link_libraries(MLIRExecutionEngine PUBLIC
MLIRTargetLLVMIRExport
)
-if(LLVM_BUILD_LLVM_DYLIB AND NOT (WIN32 OR MINGW OR CYGWIN)) # Does not build on windows currently, see #106859
+if(MLIR_BUILD_MLIR_DYLIB AND NOT (WIN32 OR MINGW OR CYGWIN)) # Does not build on windows currently, see #106859
# Build a shared library for the execution engine. Some downstream projects
# use this library to build their own CPU runners while preserving dynamic
# linkage.
diff --git a/mlir/tools/mlir-shlib/CMakeLists.txt b/mlir/tools/mlir-shlib/CMakeLists.txt
index a33c70c5807be..166bab744de36 100644
--- a/mlir/tools/mlir-shlib/CMakeLists.txt
+++ b/mlir/tools/mlir-shlib/CMakeLists.txt
@@ -30,7 +30,7 @@ if(MLIR_LINK_MLIR_DYLIB)
set(INSTALL_WITH_TOOLCHAIN INSTALL_WITH_TOOLCHAIN)
endif()
-if(LLVM_BUILD_LLVM_DYLIB)
+if(MLIR_BUILD_MLIR_DYLIB)
add_mlir_library(
MLIR
SHARED
@@ -45,6 +45,7 @@ if(LLVM_BUILD_LLVM_DYLIB)
${mlir_llvm_link_components}
)
target_link_libraries(MLIR PRIVATE ${LLVM_PTHREAD_LIB})
+ target_link_libraries(MLIRDylib INTERFACE MLIR)
endif()
#message("Libraries included in libMLIR.so: ${mlir_libs}")
|
@llvm/pr-subscribers-mlir Author: Bryan Tan (Technius) ChangesCurrently, MLIR fails to build when LLVM_BUILD_LLVM_DYLIB is OFF and MLIR_LINK_MLIR_DYLIB is ON. The reason for this is that the MLIR shared library target is only defined when LLVM_BUILD_LLVM_DYLIB is set to ON, but the Furthermore, there is a second issue where the MLIR target is referenced by To address the above problems, this commit introduces a new MLIR_BUILD_MLIR_DYLIB CMake option that is used to determine whether the MLIR shared libary target should be built. Similar to LLVM_BUILD_LLVM_DYLIB, the corresponding MLIR option is automatically enabled whenever MLIR_LINK_MLIR_DYLIB is ON. The Full diff: https://github.com/llvm/llvm-project/pull/155488.diff 4 Files Affected:
diff --git a/mlir/CMakeLists.txt b/mlir/CMakeLists.txt
index f58a4c6f506ec..371740292323a 100644
--- a/mlir/CMakeLists.txt
+++ b/mlir/CMakeLists.txt
@@ -172,8 +172,11 @@ set(MLIR_INSTALL_AGGREGATE_OBJECTS 1 CACHE BOOL
set(MLIR_BUILD_MLIR_C_DYLIB 0 CACHE BOOL "Builds libMLIR-C shared library.")
-set(MLIR_LINK_MLIR_DYLIB ${LLVM_LINK_LLVM_DYLIB} CACHE BOOL
- "Link tools against libMLIR.so")
+set(MLIR_BUILD_MLIR_DYLIB 0 CACHE BOOL "Builds the libMLIR shared library")
+set(MLIR_LINK_MLIR_DYLIB ${LLVM_LINK_LLVM_DYLIB} CACHE BOOL "Link tools against libMLIR.so")
+if (MLIR_LINK_MLIR_DYLIB)
+ set(MLIR_BUILD_MLIR_DYLIB 1)
+endif()
configure_file(
${MLIR_MAIN_INCLUDE_DIR}/mlir/Config/mlir-config.h.cmake
@@ -235,6 +238,15 @@ set(MLIR_PDLL_TABLEGEN_TARGET "${MLIR_PDLL_TABLEGEN_TARGET}" CACHE INTERNAL "")
set(MLIR_SRC_SHARDER_TABLEGEN_EXE "${MLIR_SRC_SHARDER_TABLEGEN_EXE}" CACHE INTERNAL "")
set(MLIR_SRC_SHARDER_TABLEGEN_TARGET "${MLIR_SRC_SHARDER_TABLEGEN_TARGET}" CACHE INTERNAL "")
+# Add MLIR dylib target here, as calls to mlir_target_link_libraries (used by
+# individual libs below) assume that such a target may exist,
+# but we cannot define the actual dylib until after all individual libs
+# are defined.
+if (MLIR_LINK_MLIR_DYLIB)
+ add_library(MLIRDylib INTERFACE)
+ add_mlir_library_install(MLIRDylib)
+endif()
+
add_subdirectory(include/mlir)
add_subdirectory(lib)
# C API needs all dialects for registration, but should be built before tests.
diff --git a/mlir/cmake/modules/AddMLIR.cmake b/mlir/cmake/modules/AddMLIR.cmake
index 38839679ef8b1..9545eab265db3 100644
--- a/mlir/cmake/modules/AddMLIR.cmake
+++ b/mlir/cmake/modules/AddMLIR.cmake
@@ -354,11 +354,11 @@ function(add_mlir_library name)
# Yes, because the target "obj.${name}" is referenced.
set(NEEDS_OBJECT_LIB ON)
endif ()
- if(LLVM_BUILD_LLVM_DYLIB AND NOT ARG_EXCLUDE_FROM_LIBMLIR AND NOT XCODE)
+ if(MLIR_BUILD_MLIR_DYLIB AND NOT ARG_EXCLUDE_FROM_LIBMLIR AND NOT XCODE)
# Yes, because in addition to the shared library, the object files are
# needed for linking into libMLIR.so (see mlir/tools/mlir-shlib/CMakeLists.txt).
# For XCode, -force_load is used instead.
- # Windows is not supported (LLVM_BUILD_LLVM_DYLIB=ON will cause an error).
+ # Windows is not supported (MLIR_BUILD_MLIR_DYLIB=ON will cause an error).
set(NEEDS_OBJECT_LIB ON)
set_property(GLOBAL APPEND PROPERTY MLIR_STATIC_LIBS ${name})
set_property(GLOBAL APPEND PROPERTY MLIR_LLVM_LINK_COMPONENTS ${ARG_LINK_COMPONENTS})
@@ -745,7 +745,7 @@ function(mlir_target_link_libraries target type)
endif()
if (MLIR_LINK_MLIR_DYLIB)
- target_link_libraries(${target} ${type} MLIR)
+ target_link_libraries(${target} ${type} MLIRDylib)
else()
target_link_libraries(${target} ${type} ${ARGN})
endif()
diff --git a/mlir/lib/ExecutionEngine/CMakeLists.txt b/mlir/lib/ExecutionEngine/CMakeLists.txt
index fdeb4dacf9278..85cbb1ef3bc4e 100644
--- a/mlir/lib/ExecutionEngine/CMakeLists.txt
+++ b/mlir/lib/ExecutionEngine/CMakeLists.txt
@@ -103,7 +103,7 @@ mlir_target_link_libraries(MLIRExecutionEngine PUBLIC
MLIRTargetLLVMIRExport
)
-if(LLVM_BUILD_LLVM_DYLIB AND NOT (WIN32 OR MINGW OR CYGWIN)) # Does not build on windows currently, see #106859
+if(MLIR_BUILD_MLIR_DYLIB AND NOT (WIN32 OR MINGW OR CYGWIN)) # Does not build on windows currently, see #106859
# Build a shared library for the execution engine. Some downstream projects
# use this library to build their own CPU runners while preserving dynamic
# linkage.
diff --git a/mlir/tools/mlir-shlib/CMakeLists.txt b/mlir/tools/mlir-shlib/CMakeLists.txt
index a33c70c5807be..166bab744de36 100644
--- a/mlir/tools/mlir-shlib/CMakeLists.txt
+++ b/mlir/tools/mlir-shlib/CMakeLists.txt
@@ -30,7 +30,7 @@ if(MLIR_LINK_MLIR_DYLIB)
set(INSTALL_WITH_TOOLCHAIN INSTALL_WITH_TOOLCHAIN)
endif()
-if(LLVM_BUILD_LLVM_DYLIB)
+if(MLIR_BUILD_MLIR_DYLIB)
add_mlir_library(
MLIR
SHARED
@@ -45,6 +45,7 @@ if(LLVM_BUILD_LLVM_DYLIB)
${mlir_llvm_link_components}
)
target_link_libraries(MLIR PRIVATE ${LLVM_PTHREAD_LIB})
+ target_link_libraries(MLIRDylib INTERFACE MLIR)
endif()
#message("Libraries included in libMLIR.so: ${mlir_libs}")
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple small comments, otherwise LGTM
mlir/CMakeLists.txt
Outdated
@@ -172,8 +172,11 @@ set(MLIR_INSTALL_AGGREGATE_OBJECTS 1 CACHE BOOL | |||
|
|||
set(MLIR_BUILD_MLIR_C_DYLIB 0 CACHE BOOL "Builds libMLIR-C shared library.") | |||
|
|||
set(MLIR_LINK_MLIR_DYLIB ${LLVM_LINK_LLVM_DYLIB} CACHE BOOL | |||
"Link tools against libMLIR.so") | |||
set(MLIR_BUILD_MLIR_DYLIB 0 CACHE BOOL "Builds the libMLIR shared library") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: It looks like we're propagating an ancient CMake style for declaring boolean cache variables. CMake provides a separate shorthand command that is more readable:
option(MLIR_BUILD_MLIR_DYLIB "Builds the libMLIR shared library" OFF)
"Link tools against libMLIR.so") | ||
set(MLIR_BUILD_MLIR_DYLIB 0 CACHE BOOL "Builds the libMLIR shared library") | ||
set(MLIR_LINK_MLIR_DYLIB ${LLVM_LINK_LLVM_DYLIB} CACHE BOOL "Link tools against libMLIR.so") | ||
if (MLIR_LINK_MLIR_DYLIB) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here you're setting non-cache variable MLIR_BUILD_MLIR_DYLIB
, but line 175 is populating a cache variable MLIR_BUILD_MLIR_DYLIB. Both will persist and hold different values, which can be an endless source of confusion (for me at least). Maybe other CMake users find this intuitive (that a non-cache variable overrides the cache variable of the same name if set), but I suspect most people don't know that two different kinds of variables are being created here.
Personally I think it makes more sense to force-set the cache variable
if(<condition>)
set(MLIR_BUILD_MLIR_DYLIB ON CACHE BOOL "" FORCE)
endif()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I politely disagree with the suggestion. I think the CMake documentation is quite clear on the behavior of cache variables. Forcing the value of MLIR_BUILD_MLIR_DYLIB
to ON
in the cache would be quite confusing to a user that initially sets MLIR_LINK_MLIR_DYLIB
to ON
and later turns it off.
Instead, do you think it would be better to mirror the style of how LLVM_BUILD_LLVM_DYLIB
is defined?
llvm-project/llvm/CMakeLists.txt
Lines 938 to 952 in 0b42e11
set(LLVM_BUILD_LLVM_DYLIB_default OFF) | |
if(LLVM_LINK_LLVM_DYLIB OR LLVM_BUILD_LLVM_C_DYLIB) | |
set(LLVM_BUILD_LLVM_DYLIB_default ON) | |
endif() | |
cmake_dependent_option(LLVM_BUILD_LLVM_DYLIB "Build libllvm dynamic library" ${LLVM_BUILD_LLVM_DYLIB_default} | |
"CAN_BUILD_LLVM_DYLIB" OFF) | |
cmake_dependent_option(LLVM_DYLIB_EXPORT_INLINES "Force inline members of classes to be DLL exported when | |
building with clang-cl so the libllvm DLL is compatible with MSVC" | |
OFF | |
"MSVC;LLVM_BUILD_LLVM_DYLIB_VIS" OFF) | |
if(LLVM_BUILD_LLVM_DYLIB_VIS) | |
set(LLVM_BUILD_LLVM_DYLIB ON) | |
endif() |
The name of LLVM_BUILD_LLVM_DYLIB_default
makes it clear that it is a "default value", which could avoid the confusion that you mention.
Currently, MLIR fails to build when LLVM_BUILD_LLVM_DYLIB is OFF and MLIR_LINK_MLIR_DYLIB is ON. The reason for this is that the MLIR shared library target is only defined when LLVM_BUILD_LLVM_DYLIB is set to ON, but the
mlir_target_link_libraries
CMake function will attempt to link its given target against the MLIR shared library target whenever the MLIR_LINK_MLIR_DYLIB option is ON.Furthermore, there is a second issue where the MLIR target is referenced by
mlir_target_link_libraries
calls before it is defined bytools/mlir-shlib/CMakeLists.txt
.To address the above problems, this commit introduces a new MLIR_BUILD_MLIR_DYLIB CMake option that is used to determine whether the MLIR shared libary target should be built. Similar to LLVM_BUILD_LLVM_DYLIB, the corresponding MLIR option is automatically enabled whenever MLIR_LINK_MLIR_DYLIB is ON.
The
mlir_target_link_libraries
now links against an interface target MLIRDylib, which "foward declares" the MLIR shared library target, as the latter cannot be defined until after all the individual libraries are.