Page MenuHomePhabricator

cmake: Remove 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 merges it
into add_llvm_library() and replaces all calls to add_llvm_loadable_module(lib ...)
with add_llvm_library(lib MODULE ...)

Diff Detail

Event Timeline

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

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
674

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
674

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
674

@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.

tstellar updated this revision to Diff 173490.Nov 9 2018, 9:17 PM

Remove add_llvm_loadable_module() completely.

Some of the clang examples use the add_llvm_loadable_module() macro, so I will need to update those too in a separate patch.

tstellar retitled this revision from cmake: Refactor add_llvm_loadable_module() to cmake: Remove add_llvm_loadable_module().Nov 9 2018, 9:20 PM
tstellar edited the summary of this revision. (Show Details)