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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:808
 
+if(NOT llvm-pass-plugins)
+    # Target used to hold global properties referencable from generator-expression
----------------
[serious] I get the following error:
```
CMake Error at cmake/modules/AddLLVM.cmake:813 (add_custom_target):
  add_custom_target cannot create target "llvm-pass-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/test/CMakeLists.txt:2 (include)
```

What you meant to use is
```
if (NOT TARGET llvm-pass-plugins)
```
See https://cmake.org/cmake/help/latest/command/if.html


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:851
+                "extern \"C\" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK llvmGetPassPluginInfo() { return ${entry_point}(); }")
+      target_sources(${register_llvm_pass_plugin_EXTRA_LOADABLE} PRIVATE ${CMAKE_CURRENT_BINARY_DIR}/${register_llvm_pass_plugin_EXTRA_LOADABLE}_plugin_entry_point.cpp)
+    endif()
----------------
[serious] Under Windows, I get the following error:
```
CMake Error at cmake/modules/AddLLVM.cmake:853 (target_sources):
  target_sources called with non-compilable target type
Call Stack (most recent call first):
  tools/polly/lib/CMakeLists.txt:164 (register_llvm_pass_plugin)
```

This is because "LLVMPolly" is not a library, but a dummy "add_custom_target" since loadable modules are not supported under Windows.


================
Comment at: llvm/docs/WritingAnLLVMPass.rst:1222
 
+Building out-of-tree passes
+===========================
----------------
"out-of-tree" is the wrong term. This registration only works if the plugin if configured in the same cmake-run. "out-of-tree" would describe a processes with a separate cmake source- and build-tree using `LLVM_MAIN_SRC_DIR` or https://llvm.org/docs/CMake.html#embedding-llvm-in-your-project


================
Comment at: llvm/tools/CMakeLists.txt:45
 
+add_llvm_external_project(polly)
+
----------------
What is the reason to have this after `add_llvm_implicit_projects` in contrast to the other LLVM subprojects?


================
Comment at: polly/lib/Support/RegisterPasses.cpp:727
+extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK
+llvmGetPassPluginInfo() {
+  return getPassPluginInfo();
----------------
serge-sans-paille wrote:
> Meinersbur wrote:
> > [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.
> > but linking them all statically will violate the one-definition-rule.
> 
> They are unused when liked statically, and flagged as weak to avoid link-time conflict.
> 
> > IMHO, Polly.cpp would have been a better place for this function.
> I still agree it's more explicit if linked conditionaly.
You seem to have removed the weak attribute. Did you mean to put it into the `polly` namespace to avoid name clashing with other plugins?


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

https://reviews.llvm.org/D61446