Skip to content
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

Export LLVM_VERSION_MAJOR CMake variable as a directory property #83346

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

bjacob
Copy link
Contributor

@bjacob bjacob commented Feb 28, 2024

This allows users who include llvm-project as a subrepository to access the LLVM_VERSION_MAJOR variable on par with if they were relying on an installed LLVM and FindLLVM.cmake. They just need to do something like:

  get_directory_property(LLVM_VERSION_MAJOR DIRECTORY "third_party/llvm-project/llvm" LLVM_VERSION_MAJOR)

Context: iree-org/iree#16606 -- like other projects with similar needs that I found by some googling, our work-around had been to rely on the CMake cached variable CLANG_EXECUTABLE_VERSION. Being cached, it over time inevitably ended up having a wrong value.

@bjacob bjacob marked this pull request as ready for review February 28, 2024 22:10
@bjacob bjacob force-pushed the llvm-version-major branch 3 times, most recently from 44dd7c1 to 6848a19 Compare March 1, 2024 15:54
@bjacob
Copy link
Contributor Author

bjacob commented Mar 1, 2024

Just tagged 3 tentative reviewers... feel free to reassign!

llvm/CMakeLists.txt Outdated Show resolved Hide resolved
@bjacob bjacob changed the title Export LLVM_VERSION_* CMake variables to PARENT_SCOPE Export LLVM_VERSION_MAJOR CMake variable to PARENT_SCOPE Mar 3, 2024
Copy link
Collaborator

@llvm-beanz llvm-beanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally the recommended guidance has been to either include LLVM as an ExternalProject or to import the CMake package. I mention this purely as a caution. I don't have any philosophical objection to fixing the issues related to including LLVM as a subdirectory, and generally speaking those issues are bugs.

I have two concrete suggestions below to make the code easier to read and more robust for users.

llvm/CMakeLists.txt Outdated Show resolved Hide resolved
# Export a few LLVM version identifiers for users who use LLVM as a subdir.
get_directory_property(_LLVM_HAS_PARENT_DIRECTORY PARENT_DIRECTORY)
if (_LLVM_HAS_PARENT_DIRECTORY)
set(LLVM_VERSION_MAJOR "${LLVM_VERSION_MAJOR}" PARENT_SCOPE)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more idiomatic in CMake to set those values as directory or global properties, which would allow them to be accessed at any directory scope above using the get_property function.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: If you set these as properties instead of variables, I don't think there is any need to only set them conditionally, they can just always be set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for teaching me better CMake! Now this PR is down to 1 line with no control flow and I have verified that my downstream project is able to read the variable like this,

  get_directory_property(LLVM_VERSION_MAJOR DIRECTORY "third_party/llvm-project/llvm" LLVM_VERSION_MAJOR)

@bjacob bjacob changed the title Export LLVM_VERSION_MAJOR CMake variable to PARENT_SCOPE Export LLVM_VERSION_MAJOR CMake variable as a directory property Mar 5, 2024
@bjacob
Copy link
Contributor Author

bjacob commented Mar 5, 2024

Generally the recommended guidance has been to either include LLVM as an ExternalProject or to import the CMake package.

Thanks, I didn't know about https://cmake.org/cmake/help/latest/module/ExternalProject.html . TIL. FYI @ScottTodd @stellaraccident .

@ScottTodd
Copy link
Member

Generally the recommended guidance has been to either include LLVM as an ExternalProject or to import the CMake package.

Thanks, I didn't know about https://cmake.org/cmake/help/latest/module/ExternalProject.html . TIL. FYI @ScottTodd @stellaraccident .

Neither of those are particularly attractive options when depending on many targets in the subproject (as we do with MLIR targets). If we were just using tools like lld from LLVM as black boxes and were rarely interacting with individual libraries, header files, etc. then they might work. What's sort of tricky in our (IREE's) use of LLVM/MLIR is that we use both the individual libraries in MLIR and the linking and code generation from LLVM -- and in both opinionated and general/flexible ways.

@bjacob bjacob merged commit fac791d into llvm:main Mar 5, 2024
4 checks passed
@llvm-beanz
Copy link
Collaborator

Neither of those are particularly attractive options when depending on many targets in the subproject (as we do with MLIR targets). If we were just using tools like lld from LLVM as black boxes and were rarely interacting with individual libraries, header files, etc. then they might work. What's sort of tricky in our (IREE's) use of LLVM/MLIR is that we use both the individual libraries in MLIR and the linking and code generation from LLVM -- and in both opinionated and general/flexible ways.

Yep. In the past the guidance has been for projects like that to nest as LLVM subprojects. I understand reasons why that is undesirable and welcome any contributions to make your preferred workflow better supported. I just wanted to make sure you were aware that the way you're doing things is historically fraught with peril.

bjacob added a commit to iree-org/iree that referenced this pull request Mar 14, 2024
This supersedes #16606 as the way
that we stop relying on `CLANG_EXECUTABLE_VERSION` which, as a cached
variable, is bound to eventually give an incorrect value and cause
builds to fail.

llvm/llvm-project#83346 was merged so we can now
query `LLVM_VERSION_MAJOR` as a directory property on the `llvm/`
directory.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants