Page MenuHomePhabricator

Fix interaction of static plugins with -DLLVM_LINK_LLVM_DYLIB=ON.
ClosedPublic

Authored by efriedma on Apr 16 2020, 2:36 PM.

Details

Summary

We should link static plugins into libLLVM.so; they shouldn't depend on libLLVM.so.

I'm not completely sure my CMake manipulation is right, but it seems to work.

Fixes https://bugs.llvm.org/show_bug.cgi?id=45571

Diff Detail

Event Timeline

efriedma created this revision.Apr 16 2020, 2:36 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: mgorny. · View Herald Transcript
llvm/cmake/modules/AddLLVM.cmake
886

Conceptually, do we want Extensions to be considered as component?

The cmake configure is fixed, thanks.
Running a full build now

efriedma marked an inline comment as done.Apr 17 2020, 1:32 AM
efriedma added inline comments.
llvm/cmake/modules/AddLLVM.cmake
886

An alternative way to state this question is, should LLVMLTO depend on statically linked plugins directly?

One way of thinking about it is that static plugins are a way to write LLVM components outside the llvm/ tree. Therefore, we statically link them into LTO, and use them automatically.

Alternatively, we could say that plugins are part of the calling tool. In that case, we'd need to remove most of the changes from the LTO library, add some sort of callback, and stick the code to call into the plugins into each tool that calls LTO instead.

Either way works; the end result isn't hugely different. But I think I'd prefer the approach that doesn't involve copy-pasting the plugin registration code into every tool that runs LTO.

serge-sans-paille added inline comments.
llvm/cmake/modules/AddLLVM.cmake
886

One way of thinking about it is that static plugins are a way to write LLVM components outside the llvm/ tree.

I like that perspective, it makes sense both conceptually and programmatically. LGTM

polly/lib/CMakeLists.txt
110

I'm currently working on a way to avoid the need of stating that in every plugin, this is super error-prone. It will be based on your patch.

This revision is now accepted and ready to land.Apr 17 2020, 3:52 AM
This revision was automatically updated to reflect the committed changes.