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