cmake: Refactor add_llvm_loadable_module()
Needs ReviewPublic

Authored by tstellar on Sep 6 2018, 1:24 PM.

Details

Summary

This function is very similar to add_llvm_library(), so this patch turns
add_llvm_loadable_module() into a simple wrapper around add_llvm_library().
In addition to simplifying the code, this makes it possible to mark loadable modules
as BULIDTREE_ONLY. Before this patch modules would be installed unconditionally
which is not always what we want. See https://reviews.llvm.org/D50668.

Diff Detail

tstellar created this revision.Sep 6 2018, 1:24 PM
beanz added inline comments.Sep 6 2018, 2:51 PM
cmake/modules/AddLLVM.cmake
677

I don't believe there is any situation where calling add_llvm_library with MODULE would ever result in not creating the target, so this condition is probably not needed, which makes me think we should probably just get rid of add_llvm_loadable_module entirely.

philip.pfaffe added inline comments.Sep 6 2018, 3:02 PM
cmake/modules/AddLLVM.cmake
677

Looks like it won't be created if(NOT LLVM_ENABLE_PLUGINS AND NOT (ARG_PLUGIN_TOOL AND LLVM_EXPORT_SYMBOLS_FOR_PLUGINS))

tstellar added inline comments.Sep 6 2018, 3:03 PM
cmake/modules/AddLLVM.cmake
677

What about when LLVM_ENABLE_PLUGINS is OFF? I tried a similar solution for D50668, and I thought this was why the Windows bots failed.

The other thing add_llvm_loadable_module does is:

set_target_properties(${name} PROPERTIES FOLDER "Loadable modules")

Would this be OK to drop?

chapuni added a subscriber: chapuni.Sep 7 2018, 3:36 PM
tstellar updated this revision to Diff 164543.Sep 7 2018, 4:54 PM

Fix cmake failure with -DLLVM_LINK_LLVM_DYLIB=OFF

tstellar updated this revision to Diff 164560.Sep 7 2018, 6:51 PM

Add custom install targets for modules.

beanz added inline comments.Sep 19 2018, 11:30 AM
cmake/modules/AddLLVM.cmake
677

@philip.pffae, you're right. I think if the goal is to make add_llvm_library support MODULE I still think we should have it replace add_llvm_loadable_module. Much of what is in the later is already implemented in add_llvm_library anyways (like all the install goop).

It might make sense to handle the case where the target isn't created in add_llvm_library with an early return after the llvm_add_library call that just creates the dummy target.

@tstellar, It is probably best to just move the folder setting into add_llvm_library if MODULE is set. That is really just for sorting things in the IDE generators, so it probably makes sense.