From 8d94ae74fd28c55dbd96b1569b0e41803794f4bc Mon Sep 17 00:00:00 2001
From: Christopher Bate <cbate@nvidia.com>
Date: Sat, 15 Feb 2025 22:02:17 +0000
Subject: [PATCH] [cmake] Update usage of `HandleLLVMOptions` and
 `LLVM_DEFINITIONS`

This change attempts to resolve issues with use of `HandleLLVMOptions`
and `LLVM_DEFINITIONS`, see
https://github.com/llvm/llvm-project/issues/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.
---
 CMakeLists.txt | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index bf5f017275..2a119e01df 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -109,12 +109,10 @@ if(STABLEHLO_STANDALONE_BUILD)
   set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/lib)
   list(APPEND CMAKE_MODULE_PATH "${MLIR_CMAKE_DIR}")
   list(APPEND CMAKE_MODULE_PATH "${LLVM_CMAKE_DIR}")
-  include(HandleLLVMOptions)
 endif()
 
 if(STABLEHLO_BUILD_EMBEDDED)
   message(STATUS "Building StableHLO embedded in another project")
-  include(HandleLLVMOptions)
 endif()
 
 include(TableGen)
@@ -167,14 +165,48 @@ if(STABLEHLO_ENABLE_SPLIT_DWARF)
     endif()
 endif()
 
-#TODO: Where should these be?
+# Remove these when LLVM and MLIR modernize their *Config.cmake files so that
+# compiler options are carried by target interface properties.
 include_directories(${LLVM_INCLUDE_DIRS})
 include_directories(${MLIR_INCLUDE_DIRS})
 include_directories(${CMAKE_CURRENT_SOURCE_DIR})
 include_directories(${CMAKE_CURRENT_BINARY_DIR})
 link_directories(${LLVM_BUILD_LIBRARY_DIR})
-add_definitions(${LLVM_DEFINITIONS})
 
+# Because of some subtle issues in how LLVM passes information on compilation
+# flags to downstream projects (see
+# https://github.com/llvm/llvm-project/issues/125779), there may not currently
+# be one perfect CMake incantation for setting compilation options here that
+# satisfies all potential downstream users who may depend on Stablehlo. Many
+# projects use the incantation, `find_package(LLVM...);
+# include(HandleLLVMOptions)` to try to set directory-scoped compiler flags to
+# match what was used to build LLVM. However, this is an imperfect mechanism for
+# replicating the compiler options applied to LLVM. Therefore, we should restrict
+# HandleLLVMOptions usage here to standalone mode solely for providing all the
+# `LLVM_*` CMake options which are familiar to LLVM's CMake users.
+
+# In this manner, it's always users's responsibility to ensure that Stablehlo is
+# being built with flags that are compatible with the LLVM package; e.g. both
+# projects must built with compatible flags related to RTTI and exceptions. This
+# applies regardless of whether user is building Stablehlo as the top-level
+# project or embedded within a larger build.
+if(STABLEHLO_STANDALONE_BUILD)
+  # LLVM_DEFINITIONS is a space-separated list; it must be pre-processed to use
+  # with 'add_definitions', see
+  # https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project.
+
+  # This must be sequenced prior to HandleLLVMOptions, which overwrites
+  # LLVM_DEFINITIONS. If `LLVM_DEFINITIONS` is non-empty and provided by a
+  # pre-built LLVM package's LLVMConfig.cmake, then invoking `HandleLLVMOptions`
+  # here will also cause duplication of the definitions, since the components of
+  # LLVM_DEFINITIONS are synthesized and applied directly to the current
+  # directory's definitions list inside by HandleLLVMOptions. However, this
+  # won't be a problem unless the definitions are somehow incompatible, in which
+  # case the compiler will print a macro redefinition warning.
+  separate_arguments(LLVM_DEFINITIONS_LIST NATIVE_COMMAND ${LLVM_DEFINITIONS})
+  add_definitions(${LLVM_DEFINITIONS_LIST})
+  include(HandleLLVMOptions)
+endif()
 
 #-------------------------------------------------------------------------------
 # Sanitizer configuration