Skip to content

Commit 8d94ae7

Browse files
[cmake] Update usage of HandleLLVMOptions and LLVM_DEFINITIONS
This change attempts to resolve issues with use of `HandleLLVMOptions` and `LLVM_DEFINITIONS`, see llvm/llvm-project#125779. Note that this is a breaking change because it could cause build breakage for downstream users. As noted in the comments added to the CMakeLists.txt file, there may not be one perfect CMake incantation for setting Stablehlo's options that works for all users. Since it's easier to *add* compiler options at a specific scope than it is to alter/remove options that Stablehlo itself is setting, this change is hoisting responsibility to the user for setting any compiler options previously provided by the `HandleLLVMOptions` call when building in embedded mode. This means that if user was using `FetchContent|add_subdirectory|CPMAddPackage` to build Stablehlo in their project, they should invoke ``` find_package(LLVM CONFIG REQUIRED) separate_arguments(LLVM_DEFINITIONS_LIST NATIVE_COMMAND ${LLVM_DEFINITIONS}) add_definitions(${LLVM_DEFINITIONS_LIST}) include(HandleLLVMOptions) ``` in their project at the appropriate scope, or set desired flags in some other manner.
1 parent 4598975 commit 8d94ae7

File tree

1 file changed

+36
-4
lines changed

1 file changed

+36
-4
lines changed

CMakeLists.txt

Lines changed: 36 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -109,12 +109,10 @@ if(STABLEHLO_STANDALONE_BUILD)
109109
set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/lib)
110110
list(APPEND CMAKE_MODULE_PATH "${MLIR_CMAKE_DIR}")
111111
list(APPEND CMAKE_MODULE_PATH "${LLVM_CMAKE_DIR}")
112-
include(HandleLLVMOptions)
113112
endif()
114113

115114
if(STABLEHLO_BUILD_EMBEDDED)
116115
message(STATUS "Building StableHLO embedded in another project")
117-
include(HandleLLVMOptions)
118116
endif()
119117

120118
include(TableGen)
@@ -167,14 +165,48 @@ if(STABLEHLO_ENABLE_SPLIT_DWARF)
167165
endif()
168166
endif()
169167

170-
#TODO: Where should these be?
168+
# Remove these when LLVM and MLIR modernize their *Config.cmake files so that
169+
# compiler options are carried by target interface properties.
171170
include_directories(${LLVM_INCLUDE_DIRS})
172171
include_directories(${MLIR_INCLUDE_DIRS})
173172
include_directories(${CMAKE_CURRENT_SOURCE_DIR})
174173
include_directories(${CMAKE_CURRENT_BINARY_DIR})
175174
link_directories(${LLVM_BUILD_LIBRARY_DIR})
176-
add_definitions(${LLVM_DEFINITIONS})
177175

176+
# Because of some subtle issues in how LLVM passes information on compilation
177+
# flags to downstream projects (see
178+
# https://github.com/llvm/llvm-project/issues/125779), there may not currently
179+
# be one perfect CMake incantation for setting compilation options here that
180+
# satisfies all potential downstream users who may depend on Stablehlo. Many
181+
# projects use the incantation, `find_package(LLVM...);
182+
# include(HandleLLVMOptions)` to try to set directory-scoped compiler flags to
183+
# match what was used to build LLVM. However, this is an imperfect mechanism for
184+
# replicating the compiler options applied to LLVM. Therefore, we should restrict
185+
# HandleLLVMOptions usage here to standalone mode solely for providing all the
186+
# `LLVM_*` CMake options which are familiar to LLVM's CMake users.
187+
188+
# In this manner, it's always users's responsibility to ensure that Stablehlo is
189+
# being built with flags that are compatible with the LLVM package; e.g. both
190+
# projects must built with compatible flags related to RTTI and exceptions. This
191+
# applies regardless of whether user is building Stablehlo as the top-level
192+
# project or embedded within a larger build.
193+
if(STABLEHLO_STANDALONE_BUILD)
194+
# LLVM_DEFINITIONS is a space-separated list; it must be pre-processed to use
195+
# with 'add_definitions', see
196+
# https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project.
197+
198+
# This must be sequenced prior to HandleLLVMOptions, which overwrites
199+
# LLVM_DEFINITIONS. If `LLVM_DEFINITIONS` is non-empty and provided by a
200+
# pre-built LLVM package's LLVMConfig.cmake, then invoking `HandleLLVMOptions`
201+
# here will also cause duplication of the definitions, since the components of
202+
# LLVM_DEFINITIONS are synthesized and applied directly to the current
203+
# directory's definitions list inside by HandleLLVMOptions. However, this
204+
# won't be a problem unless the definitions are somehow incompatible, in which
205+
# case the compiler will print a macro redefinition warning.
206+
separate_arguments(LLVM_DEFINITIONS_LIST NATIVE_COMMAND ${LLVM_DEFINITIONS})
207+
add_definitions(${LLVM_DEFINITIONS_LIST})
208+
include(HandleLLVMOptions)
209+
endif()
178210

179211
#-------------------------------------------------------------------------------
180212
# Sanitizer configuration

0 commit comments

Comments
 (0)