Meinersbur requested changes to this revision. Meinersbur added a comment. This revision now requires changes to proceed.
In D61446#1533076 <https://reviews.llvm.org/D61446#1533076>, @serge-sans-paille wrote: > That's what I tried to do when always adding Polly.cpp to PollyCore, but it > doesn't seem to be enough, I still need to invstigate why (it is actually > enough when using BUILD_SHARED_LIBS=ON) With static libraries, you had the following structure: PollyCore.a: RegisterPasses.o Polly.o <-- static initializer here, but never used LLVMPolly.so (for use with -load mechanism) RegisterPasses.o Polly.o <-- static initializer here, executed when loading the .so opt: main.o <-- call to initializePollyPass removed With static linking, there is no reason for the linker to add Polly.o to the `opt` executable because no symbol from Polly.o (or any of PollyCore) was required to resolve any symbol in `opt`. Building a shared library using the object library files will include all object files, since it does not know which symbols will be used at runtime. I recommend reading http://www.lurklurk.org/linkers/linkers.html#staticlibs and https://www.bfilipek.com/2018/02/static-vars-static-lib.html. What I proposed was PollyCore.a: RegisterPasses.o LLVMPolly.so (for use with -load mechanism) RegisterPasses.o Polly.o opt: // or any ENABLE_PLUGINS tool main.o Polly.o // via add_executable(opt Polly.cpp) or target_sources Adding the object file explicitly to the add_executable will add it unconditionally, even if no none of its symbol is required, like for LLVMPolly.so. In the latest update you changed the location of the static initalizer to `RegisterPasses.o`, which contains the symbol `llvmGetPassPluginInfo` which is pulled-in by the new pass manager plugin system. This makes an unfortunate dependence of the static initializer on the `llvmGetPassPluginInfo` mechanism (which I think only works for `opt` in the current patch). There is a reason why pass loading in Polly is organized as it is. ================ Comment at: llvm/cmake/modules/AddLLVM.cmake:812 +# llvm::PassPluginLibraryInfo ${entry_point}(); +add_custom_target(LLVM_PLUGINS) # target used to hold global properties referencable from generator-expression +function(register_llvm_extension llvm_extension entry_point) ---------------- beanz wrote: > Change this to `llvm-plugins` to match our convention and wrap it in `if (NOT > TARGET...)` so it doesn't error if AddLLVM is included twice. [serious] I get multiple of these errors by cmake: ``` CMake Error at cmake/modules/AddLLVM.cmake:812 (add_custom_target): add_custom_target cannot create target "LLVM_PLUGINS" because another target with the same name already exists. The existing target is a custom target created in source directory "/home/meinersbur/src/llvm". See documentation for policy CMP0002 for more details. Call Stack (most recent call first): projects/compiler-rt/unittests/CMakeLists.txt:2 (include) ``` ================ Comment at: llvm/tools/opt/NewPMDriver.cpp:292 +#define HANDLE_EXTENSION(Ext) \ + get##Ext##PluginInfo().RegisterPassBuilderCallbacks(PB); +#include "llvm/Support/Extension.def" ---------------- [serious] This will pull-in the symbol `llvmGetPassPluginInfo` from the plugin for `opt`, but what about the other tools (bugpoint/clang)? ================ Comment at: polly/lib/Support/RegisterPasses.cpp:727 +extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK +llvmGetPassPluginInfo() { + return getPassPluginInfo(); ---------------- [serious] Unfortunately, the new pass manager's plugin system relies on the function name to be `llvmGetPassPluginInfo` in each plugin. This works with multiple dynamic libraries all declaring the same name using the `PassPlugin::Load` mechanism, but linking them all statically will violate the one-definition-rule. IMHO, Polly.cpp would have been a better place for this function. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61446/new/ https://reviews.llvm.org/D61446 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
