-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
CMake build optimization #4507
CMake build optimization #4507
Conversation
Dependencies are still linked through usage requirements.
@@ -313,11 +313,7 @@ if(NOT LLVM_FOUND) | |||
else() | |||
add_definitions(${LLVM_DEFINITIONS}) | |||
add_definitions(-DLLVM_AVAILABLE) | |||
if(CMAKE_BUILD_TYPE STREQUAL "Release") | |||
llvm_map_components_to_libnames(LLVM_LIBS core support executionengine object runtimedyld x86desc x86info scalaropts mcjit vectorize ipo x86codegen x86disassembler) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You most likely didn't link the libraries in the right order previously or were missing some targets. This is using the main targets defined by the LLVM CMake build system and should be correct.
It will pull the right dependencies in the right order on the linker command line.
else() | ||
llvm_map_components_to_libnames(LLVM_LIBS core support executionengine object runtimedyld x86desc x86info scalaropts mcjit vectorize ipo x86codegen x86disassembler mcdisassembler) | ||
endif() | ||
set(LLVM_LIBS LLVMMCJIT LLVMX86CodeGen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set
is archaic and could break. Why not continue using llvm_map_components_to_libnames
? At least that's the suggested usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can it break?
The targets of the libraries are exactly what I wrote. They're not going to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Orphis If they change the name (depending on the build config) or something. Better use the provided function without explicit reason not to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's the name of the target, not the name of the build artifact. Target names are stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, it might work but why change from officially supported method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's only a method that's used to map llvm-config components to CMake targets.
Nowhere does it say you can't use the CMake targets directly. Lots of projects are integrating with the LLVM codebase and they have to use those targets.
Renaming those for no reason would break them and they don't want to do that, so it's a part of the API now.
Marks all the 3rd party dependencies with EXCLUDE_FROM_ALL, so they will only be built if requested.
Links only against the main top level LLVM targets, which avoids linking against unneeded LLVM targets.
Those 2 changes make the full build (with LLVM) compile about 20% less files.