[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.

Windows seems to work. Good job!

Linux works with static libraries, but not with `BUILD_SHARED_LIBS=ON`:

  $ bin/opt
  : CommandLine Error: Option 'polly-dependences-computeout' registered more than once!
  LLVM ERROR: inconsistency in registered CommandLine options

This error is typical for having translation units (in this case: Polly's `DependenceInfo.cpp`) multiple times in the address space.

See https://groups.google.com/forum/#!topic/polly-dev/vxumPMhrSEs for the configurations I intend to test and which currently work.



================
Comment at: llvm/cmake/modules/AddLLVM.cmake:815
+function(add_llvm_pass_plugin name)
+    cmake_parse_arguments(ARG "" "" "" ${ARGN})
+
----------------
Why use `cmake_parse_arguments` if there are no options to parse?


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:825
+    set_property(GLOBAL APPEND PROPERTY LLVM_COMPILE_EXTENSIONS ${name})
+    get_property(llvm_plugin_targets GLOBAL PROPERTY LLVM_PLUGIN_TARGETS)
+    foreach(llvm_plugin_target ${llvm_plugin_targets})
----------------
[concern] This requires that all plugin-able tool have been registered before. It might be confusing that not all plugins will be loaded into every tool if the relative order is not <all add_llvm_executable(... ENABLE_PLUGINS)> <all add_llvm_pass_plugin(...)>.

Is it possible to error-out if a ENABLE_PLUGINS happens after a plugin has been added via add_llvm_pass_plugin?


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:827
+    foreach(llvm_plugin_target ${llvm_plugin_targets})
+      set_property(TARGET ${llvm_plugin_target} APPEND PROPERTY SOURCES $<TARGET_OBJECTS:obj.${name}>)
+      set_property(TARGET ${llvm_plugin_target} APPEND PROPERTY LINK_LIBRARIES ${name})
----------------
This injects all plugin sources directly into tool executable (in addition to loading them as a library with `LINK_LIBRARIES`), probably the reason for the error I see with `BUILD_SHARED_LIBS`.

It ignores the library separation, which is not a nice solution for the same reasons why LLVM does not simply add all object files from all its libraries to each of the add_llvm_executables, but instead uses `target_link_libraries`.


================
Comment at: llvm/cmake/modules/LLVMProcessSources.cmake:112
           endif()
+          message(STATUS "${listed} gp:${gp}")
           message(SEND_ERROR "Found unknown source file ${fn_relative}
----------------
[nit] unrelated?


================
Comment at: llvm/docs/WritingAnLLVMPass.rst:1240
+
+Out-of-tree passes are compiled and link statically by default, but it's
+possible to set the following variables to change this behavior:
----------------
"Out-of-tree" still mentioned here.


================
Comment at: polly/lib/Support/RegisterPasses.cpp:727
+extern "C" ::llvm::PassPluginLibraryInfo LLVM_ATTRIBUTE_WEAK
+llvmGetPassPluginInfo() {
+  return getPassPluginInfo();
----------------
serge-sans-paille wrote:
> 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)
Unfortunately, the Windows platform has no concept of weak symbols.

`get##name##PluginInfo` is not related to the legacy pass manager. The legacy passe manager uses `llvm::PassRegistry` and `llvm::RegisterStandardPasses`. `polly::RegisterPollyPasses` is only used by the new pass manager.

Could you create a second plugin using `add_llvm_pass_plugin`, for instance convert `LLVMHello`? Then we could check whether this works.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446