Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly.

[DiamondLovesYou]

Summary

clang doesn't need to link Polly when built with LLVM_LINK_LLVM_DYLIB.

[DiamondLovesYou]

• Fix cmake warning

[beanz]

I don’t think this is the right solution. The build system knows what components are in the dylib and should remove them from the list of libraries linked individually. You should be able to make Polly behave like an LLVM component, then tools don’t need to care if the dylib is used or not.

[DiamondLovesYou]

In D51986#1232320, @beanz wrote:

I don’t think this is the right solution. The build system knows what components are in the dylib and should remove them from the list of libraries linked individually. You should be able to make Polly behave like an LLVM component, then tools don’t need to care if the dylib is used or not.

That's not true if target_link_libraries(${target} _whatever_ ${libs}) is used. That function will pull in all of each of ${libs}' dependencies, which causes problems because then, eg, bugpoint will then be linked with libLLVM.so (as expected) AND libPolly.a (assuming BUILD_SHARED_MODULES is false) AND then a bunch of LLVM components. The LLVM components (including Polly) will then conflict with libLLVM.so.

Polly already enjoys status as an LLVM component. No effort was necessary to include Polly in libLLVM.so for example.

[DiamondLovesYou]

In D51986#1232419, @DiamondLovesYou wrote:

In D51986#1232320, @beanz wrote:

I don’t think this is the right solution. The build system knows what components are in the dylib and should remove them from the list of libraries linked individually. You should be able to make Polly behave like an LLVM component, then tools don’t need to care if the dylib is used or not.

That's not true if target_link_libraries(${target} _whatever_ ${libs}) is used. That function will pull in all of each of ${libs}' dependencies, which causes problems because then, eg, bugpoint will then be linked with libLLVM.so (as expected) AND libPolly.a (assuming BUILD_SHARED_MODULES is false) AND then a bunch of LLVM components. The LLVM components (including Polly) will then conflict with libLLVM.so.

Polly already enjoys status as an LLVM component. No effort was necessary to include Polly in libLLVM.so for example.

To add: is there a why to specify optional (as in, link if present) components?

[beanz]

After line 18 in this file you could do something like:

if(WITH_POLLY)
list(APPEND LLVM_LINK_COMPONENTS Polly)
endif()

Then you can get rid of the target_link_libraries call.

[DiamondLovesYou]

In D51986#1232440, @beanz wrote:

After line 18 in this file you could do something like:

if(WITH_POLLY)
list(APPEND LLVM_LINK_COMPONENTS Polly)
endif()

Then you can get rid of the target_link_libraries call.

Ah, indeed, thanks. I'll update the other patch too.

[DiamondLovesYou]

In D51986#1232440, @beanz wrote:

After line 18 in this file you could do something like:

if(WITH_POLLY)
list(APPEND LLVM_LINK_COMPONENTS Polly)
endif()

Then you can get rid of the target_link_libraries call.

It turns out this can't be done. Consider the LLVMLTO component and its loadable form, LTO. Contrast this with Polly: Polly is the component and LLVMPolly is the loadable module.
This naming convention disagreement prevents Polly's use as a component while also not breaking LTO, I've found (beanz, is this what you were referring to?).
Specifically, CMake fails because executables can't link to a MODULE_LIBRARY target when trying to link LLVMPolly.

I think the Polly naming scheme should be changed to match LTO's scheme, rather than vice versa, but not my call.

[beanz]

I agree that changing the naming scheme probably makes sense, but I also think that you can probably make a fairly well contained change to the LLVM CMake module that maps components to libnames to special case Polly and make this all work.

I’d really like to see us avoid needing special casing for LLVM_LINK_LLVM_DYLIB as much as possible because it really shouldn’t matter to the tool where the LLVM library functionality comes from as long as it is there.

tools/driver/CMakeLists.txt

if(CLANG_ORDER_FILE AND
   if("${ORDER_FILE}" STREQUAL "\n")
     set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS ${CLANG_ORDER_FILE})
   elseif(LINKER_ORDER_FILE_WORKS)
     target_link_libraries(clang PRIVATE ${LINKER_ORDER_FILE_OPTION})
     set_target_properties(clang PROPERTIES LINK_DEPENDS ${CLANG_ORDER_FILE})
   endif()
 endif()
 
-if(WITH_POLLY AND LINK_POLLY_INTO_TOOLS)
+if(WITH_POLLY AND LINK_POLLY_INTO_TOOLS AND NOT LLVM_LINK_LLVM_DYLIB)
   target_link_libraries(clang PRIVATE Polly)
-endif(WITH_POLLY AND LINK_POLLY_INTO_TOOLS)
+endif()