Page MenuHomePhabricator

cmake: Remove add_llvm_loadable_module()
ClosedPublic

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

Repository
rL LLVM

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
677 ↗(On Diff #164276)

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 ↗(On Diff #164276)

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 ↗(On Diff #164276)

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 ↗(On Diff #164276)

@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)
philip.pfaffe accepted this revision.Dec 20 2018, 2:28 AM

This looks mostly like a mechanical change now, so LGTM!

This revision is now accepted and ready to land.Dec 20 2018, 2:28 AM
This revision was automatically updated to reflect the committed changes.

this change broken mingw build.

CMake Error at cmake/modules/AddLLVM.cmake:667 (install):
  install TARGETS given no LIBRARY DESTINATION for module target
  "BugpointPasses".
Call Stack (most recent call first):
  tools/bugpoint-passes/CMakeLists.txt:17 (add_llvm_library)


CMake Error at cmake/modules/AddLLVM.cmake:667 (install):
  install TARGETS given no LIBRARY DESTINATION for module target
  "TestPlugin".
Call Stack (most recent call first):
  unittests/Passes/CMakeLists.txt:18 (add_llvm_library)
hans added a subscriber: hans.Dec 21 2018, 1:14 AM

Maybe add a release note for out-of-tree users of this? We had to adjust our clang plugin build files in Chromium for example.

Maybe add a release note for out-of-tree users of this? We had to adjust our clang plugin build files in Chromium for example.

I've add this to the release notes in r349921.