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