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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
