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

Use cmake find_package to find LLVM rather than a custom setup #1279

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

debaetsd
Copy link
Contributor

Description

This PR changes how CMake searches for LLVM.
Rather then having a custom FindLLVM setup, it is possible to rely on importable targets setup by the LLVM project.

This was proposed/discussed in LLVM importable CMake targets on the mailing list (initial post on Oct 16 2020).

Please note that I renamed the clang-format target into osl-clang-format to prevent name clashes with the "real" clang-format.

Tests

Existing CI still passes.
Windows build verified locally.
Updated CI to look for osl-clang-format

Checklist:

  • I have read the contribution guidelines.
  • I have previously submitted a Contributor License Agreement.
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

Copy link
Collaborator

@lgritz lgritz left a comment

Choose a reason for hiding this comment

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

I like where this is going! It will be great to finally get rid of our FindLLVM.cmake, which I think we wrote in the early LLVM 3 days, long before LLVM contained exported config files. I pose some comments/questions for you to consider.

@@ -469,7 +469,7 @@ if (CLANG_FORMAT_EXE)
# message (STATUS "clang-format file list: ${FILES_TO_FORMAT}")
file (COPY ${CMAKE_CURRENT_SOURCE_DIR}/.clang-format
DESTINATION ${CMAKE_CURRENT_BINARY_DIR})
add_custom_target (clang-format
add_custom_target (osl-clang-format
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you don't mind, let's call this "run-clang-format".

I tend to copy and/or synchronize the more generic cmake scripts between multiple projects, so it's very handy to not have too many project-specific names littering the code like this that could in theory be applied to any project.

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 have zero preference in this so run-clang-format is fine

Comment on lines +113 to +115
# libclang libraries statically. So on apple and when LLVM 10+ are involved,
# just force that choice. Other than larger executables, it seems harmless,
# and in any case a better choice than this beastly bug.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, have you checked that this is still necessary in the new LLVM 11?

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 have not tested this on Mac (and just relied on the github CI).
I could set up a build environment on my macbook but it will take some time (I don't even have brew installed :) ). Perhaps somebody else in the OSL community can help test this quicker ?

Comment on lines +131 to +133
include_directories(BEFORE SYSTEM ${LLVM_INCLUDE_DIRS})
add_definitions(${LLVM_DEFINITIONS})
link_directories(${LLVM_LIBRARY_DIRS})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these still necessary to be declared here and therefore be exposed to the whole of the osl code base? Or are exported targets set up correctly by the LLVM config cmake? In other words, should these be deleted, and instead liboslexec and other components should have target_link_libaries say PRIVATE llvm::foo for whatever specific components they use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kinda, for the clang libraries this is trivial. For LLVM itself, it is a bit more complicated.
It seems that OSL depends on 2 target independent libs (MCJIT and Passes) and 3 target dependent (Codegen, Disassembler, AsmParser).
For the independent once, it is trivial to target them directly.
For the target dependent libs, it is a bit harder as they contain the target in the lib name (for example LLVMX86CodeGen).
At first I just took the route of adding them explicit, but later reverted to the "target all what the installed llvm version supports".
That is why I use the llvm_map_components_to_libnames in order to support arbitrary targets.
So I guess the question is, does OSL support anything other than x86 and NVPTX ?
Ff not, it is trivial to directly target those components.
If we want to support all possible targets we needs some additional setup (though we could probably limit it to oslexec). Note that I think it can be further simplified upon bumping the minimum required version to LLVM8 as then have all the special allXXX targets.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, OSL doesn't support anything but x86 and NVPTX, but I expect it will need to fairly soon (at the very least, the ARM-based Macs that are coming.

@lgritz
Copy link
Collaborator

lgritz commented Oct 28, 2020

FYI, I'm fiddling around with this to get it totally working with the GPU mode and on my Mac. There are some minor edits I'll need to push to amend it.

@debaetsd
Copy link
Contributor Author

@lgritz I hacked up a test to use target_link_libaries directly: debaetsd@0031688 All in all pretty clean imho. This will work for any target (installed in the linked LLVM that is). Note though that this has issues with llvm7, either we need to special case it or we drop support for it.

@lgritz
Copy link
Collaborator

lgritz commented Jan 5, 2022

I think we let this languish for a while because it didn't work for llvm 7, which we were still supporting at the time. Since then, we have pulled (in main anyway) the minimum llvm to 9, so we should see if we can revive this.

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.

2 participants