[PATCH] D78332: Fix interaction of static plugins with -DLLVM_LINK_LLVM_DYLIB=ON.

[serge via Phabricator via llvm-commits]

serge-sans-paille accepted this revision.
serge-sans-paille added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:886
     # object library as of now
-    add_llvm_library(${name} OBJECT ${ARG_UNPARSED_ARGUMENTS})
+    add_llvm_component_library(${name} OBJECT ${ARG_UNPARSED_ARGUMENTS})
     target_compile_definitions(${name} PRIVATE LLVM_${name_upper}_LINK_INTO_TOOLS)
----------------
efriedma wrote:
> serge-sans-paille wrote:
> > Conceptually, do we want Extensions to be considered as component?
> 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.
> 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


================
Comment at: polly/lib/CMakeLists.txt:110
 
-if (LLVM_LINK_LLVM_DYLIB)
+if (LLVM_LINK_LLVM_DYLIB AND NOT LLVM_POLLY_LINK_INTO_TOOLS)
   # The shlib/dylib contains all the LLVM components
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78332/new/

https://reviews.llvm.org/D78332