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

[serge via Phabricator via cfe-commits]

serge-sans-paille marked 10 inline comments as done.
serge-sans-paille 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
----------------
Meinersbur wrote:
> [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
I'm no longer using that mechanism, it was showing some limits wrt. cmake generator-expression and export set. 


================
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()
----------------
Meinersbur wrote:
> [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.
I'm no longer using `target_sources`, it should be fine now.


================
Comment at: llvm/docs/WritingAnLLVMPass.rst:1222
 
+Building out-of-tree passes
+===========================
----------------
Meinersbur wrote:
> "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
I'm not super happy with the new title, but I gave it a try.


================
Comment at: llvm/tools/CMakeLists.txt:45
 
+add_llvm_external_project(polly)
+
----------------
Meinersbur wrote:
> What is the reason to have this after `add_llvm_implicit_projects` in contrast to the other LLVM subprojects?
polly used to be included before the other external projects, so that clang/opt could depend on it. We are now injecting dependencies a posteriori, so  polly needs to be included after opt/clang/bugpoint. I've put it right before the loop on `add_llvm_external_project` because they share the same function call name.


================
Comment at: polly/lib/Support/RegisterPasses.cpp:727
+extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK
+llvmGetPassPluginInfo() {
+  return getPassPluginInfo();
----------------
Meinersbur wrote:
> 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?
There are two entry points: `llvmGetPassPluginInfo`  for new PM (marked as weak) and `get##name##PluginInfo` for legacy PM (name is supposed to be unique, no need for weak linkage)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446