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.

Sign up for GitHub

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

Jump to bottom

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
+52 −162
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,7 +356,7 @@ jobs:
- uses: actions/checkout@v2
- name: all
env:
BUILDTARGET: clang-format
BUILDTARGET: osl-clang-format
CMAKE_CXX_STANDARD: 17
PYTHON_VERSION: 3.7
SKIP_TESTS: 1
Expand Down
2 changes: 1 addition & 1 deletion src/cmake/compiler.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

COMMAND "${CLANG_FORMAT_EXE}" -i -style=file ${FILES_TO_FORMAT} )
else ()
message (STATUS "clang-format not found.")
Expand Down
58 changes: 50 additions & 8 deletions src/cmake/externalpackages.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,19 +100,59 @@ checked_find_package (pugixml 1.8 REQUIRED)


# LLVM library setup
checked_find_package (LLVM 7.0 REQUIRED
PRINT LLVM_SYSTEM_LIBRARIES CLANG_LIBRARIES)
# ensure include directory is added (in case of non-standard locations
include_directories (BEFORE SYSTEM "${LLVM_INCLUDES}")
link_directories ("${LLVM_LIB_DIR}")

############ HACK ##############
# On OSX, the Homebrew (and maybe any build) of LLVM 10.0 seems to have a
# link conflict with its dependency on the llvm libc++ and the system
# libc++, both can end up dynamically linked and lead to very subtle and
# frustrating behavior failures (in particular, osl's use of libclang will
# botch include file parsing any time LD_LIBRARY_PATH doesn't have the llvm
# libc++ first).
#
# It seems that this is not a problem when linking against the llvm and
# 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.
Comment on lines +113 to +115
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 ?

#
# We can periodically revisit this with new version of LLVM, maybe they will
# fix things and we won't require this preemptive static linking.
if (APPLE AND LLVM_VERSION VERSION_GREATER_EQUAL 10.0)
set (LLVM_STATIC ON)
endif ()

checked_find_package(LLVM REQUIRED CONFIG
HINTS ${LLVM_DIRECTORY})

# manual version check because "LLVM is API-compatible only with matching major.minor versions" aka it requires VERSION_EQUAL
if ( ${LLVM_PACKAGE_VERSION} VERSION_LESS 7 )
message(FATAL_ERROR "LLVM 7.0+ is required but ${LLVM_PACKAGE_VERSION} was found")
endif()

include_directories(BEFORE SYSTEM ${LLVM_INCLUDE_DIRS})
add_definitions(${LLVM_DEFINITIONS})
link_directories(${LLVM_LIBRARY_DIRS})
Comment on lines +131 to +133
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.


llvm_map_components_to_libnames(LLVM_LIBRARIES
MCJIT Passes
${LLVM_TARGETS_TO_BUILD}) #this will link with all LLVM targets (x86, nvptx, ...) specific libraries (codegen, dissasembly,...)

# Extract and concatenate major & minor, remove wayward patches,
# dots, and "svn" or other suffixes.
string (REGEX REPLACE "([0-9]+)\\.([0-9]+).*" "\\1\\2" OSL_LLVM_VERSION ${LLVM_VERSION})
string (REGEX REPLACE "([0-9]+)\\.([0-9]+).*" "\\1\\2" OSL_LLVM_VERSION ${LLVM_PACKAGE_VERSION})
add_definitions (-DOSL_LLVM_VERSION=${OSL_LLVM_VERSION})
add_definitions (-DOSL_LLVM_FULL_VERSION="${LLVM_VERSION}")
add_definitions (-DOSL_LLVM_FULL_VERSION="${LLVM_PACKAGE_VERSION}")
if (LLVM_NAMESPACE)
add_definitions ("-DLLVM_NAMESPACE=${LLVM_NAMESPACE}")
add_definitions ("-DLLVM_NAMESPACE=${LLVM_NAMESPACE}")
endif ()
if (NOT LLVM_DIRECTORY)
set(LLVM_DIRECTORY ${LLVM_INSTALL_PREFIX})
endif()
set(LLVM_TARGETS ${LLVM_TARGETS_TO_BUILD})

checked_find_package(Clang REQUIRED)
include_directories(BEFORE SYSTEM ${CLANG_INCLUDE_DIRS})
set(CLANG_LIBRARIES clangCrossTU) # TODO figure out what exactly we need (this seems to work though it might be overkill)

if (LLVM_VERSION VERSION_GREATER_EQUAL 10.0.0 AND
CMAKE_CXX_STANDARD VERSION_LESS 14)
message (FATAL_ERROR
Expand All @@ -135,6 +175,8 @@ if (APPLE AND LLVM_VERSION VERSION_EQUAL 10.0.1 AND EXISTS "/usr/local/Cellar/ll
"${ColorReset}\n")
endif ()


# partio
checked_find_package (partio)


Expand Down
152 changes: 0 additions & 152 deletions src/cmake/modules/FindLLVM.cmake
View file
Open in desktop

This file was deleted.