[cmake] Update usage of HandleLLVMOptions and LLVM_DEFINITIONS#2714
[cmake] Update usage of HandleLLVMOptions and LLVM_DEFINITIONS#2714christopherbate wants to merge 1 commit intoopenxla:mainfrom
HandleLLVMOptions and LLVM_DEFINITIONS#2714Conversation
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.
|
This may break some downstream builds, I believe torch-mlir may use that embedded build? #2572 had some discussion on this |
|
Right it hoists some responsibility to the parent project in embedded mode. I can test to see if they would need to make a change or not. Perhaps I can factor out the part about LLVM_DEFINITION, which is correcting a bug, and retain existing behavior with regard to HandleLLVMOptions? |
That would be great actually. All for hoisting that responsibility as long as its known to people building it that they'll need to hoist
Either works! |
|
Any updates here? From the last comments it seems like there was more action needed before merge? |
|
Sorry I've been swamped and haven't had a chance to check Torch-MLIR's build. I'll give it a try tomorrow |
2 participants
This change attempts to resolve issues with use of
HandleLLVMOptionsandLLVM_DEFINITIONS, seellvm/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
HandleLLVMOptionscall when building in embedded mode.This means that if user was using
FetchContent|add_subdirectory|CPMAddPackageto build Stablehlo in their project, they should invokein their project at the appropriate scope, or set desired flags in some other manner.