[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

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