Page MenuHomePhabricator

Fixes for `LLVM_LINK_LLVM_DYLIB` && Polly.
Needs ReviewPublic

Authored by DiamondLovesYou on Sep 12 2018, 7:23 AM.

Details

Reviewers
beanz
bollu
Summary

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

Diff Detail

Event Timeline

  • Fix cmake warning

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.

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.

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 added a comment.Sep 12 2018, 1:57 PM

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.

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 added a comment.EditedSep 12 2018, 4:20 PM

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 added a comment.Sep 12 2018, 5:11 PM

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.

mati865 added a subscriber: mati865.Apr 8 2019, 3:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 8 2019, 3:13 AM