Skip to content

Conversation

Technius
Copy link
Contributor

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.

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.
Copy link

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 @ followed by their GitHub username.

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.

@llvmbot
Copy link
Member

llvmbot commented Aug 26, 2025

@llvm/pr-subscribers-mlir-core

Author: Bryan Tan (Technius)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/155488.diff

4 Files Affected:

  • (modified) mlir/CMakeLists.txt (+14-2)
  • (modified) mlir/cmake/modules/AddMLIR.cmake (+3-3)
  • (modified) mlir/lib/ExecutionEngine/CMakeLists.txt (+1-1)
  • (modified) mlir/tools/mlir-shlib/CMakeLists.txt (+2-1)
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}")

@llvmbot
Copy link
Member

llvmbot commented Aug 26, 2025

@llvm/pr-subscribers-mlir-execution-engine

Author: Bryan Tan (Technius)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/155488.diff

4 Files Affected:

  • (modified) mlir/CMakeLists.txt (+14-2)
  • (modified) mlir/cmake/modules/AddMLIR.cmake (+3-3)
  • (modified) mlir/lib/ExecutionEngine/CMakeLists.txt (+1-1)
  • (modified) mlir/tools/mlir-shlib/CMakeLists.txt (+2-1)
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}")

@llvmbot
Copy link
Member

llvmbot commented Aug 26, 2025

@llvm/pr-subscribers-mlir

Author: Bryan Tan (Technius)

Changes

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.


Full diff: https://github.com/llvm/llvm-project/pull/155488.diff

4 Files Affected:

  • (modified) mlir/CMakeLists.txt (+14-2)
  • (modified) mlir/cmake/modules/AddMLIR.cmake (+3-3)
  • (modified) mlir/lib/ExecutionEngine/CMakeLists.txt (+1-1)
  • (modified) mlir/tools/mlir-shlib/CMakeLists.txt (+2-1)
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}")

Copy link
Contributor

@christopherbate christopherbate left a 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

@@ -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")
Copy link
Contributor

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)
Copy link
Contributor

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()

Copy link
Contributor Author

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?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants