Page MenuHomePhabricator

Generalize the pass registration mechanism used by Polly to any third-party tool
Needs ReviewPublic

Authored by serge-sans-paille on May 2 2019, 9:11 AM.

Details

Summary

There's quite a lot of references to Polly in the LLVM CMake codebase. However the registration pattern used by Polly could be useful to other external projects: thanks to that mechanism it would be possible to develop LLVM extension without touching the LLVM code base.

This an attempt at doing so, using Polly as reference.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
serge-sans-paille marked 3 inline comments as done.May 8 2019, 12:13 AM
serge-sans-paille added inline comments.
CMakeLists.txt
521 ↗(On Diff #198012)

There was indeed a way :-)

serge-sans-paille marked an inline comment as done.May 9 2019, 1:07 AM

@mehdi_amini I've updated the .tgz sample so that one could see that the expected CMakelists.txt for third-party compiler extensions is now super-simple.

Wow that's much simpler!

CMakeLists.txt
1124 ↗(On Diff #198588)

What is xfor file by the way?

Also can you expand on the documentation here? (what is the file containing, etc.)

1125 ↗(On Diff #198588)

Where is this property populated?

cmake/modules/AddLLVM.cmake
811 ↗(On Diff #198588)

Can you add a comment explaining what you're doing here?
(it is easy to figure in the context of this diff, but it may not be obvious from the point of view of the reader out-of-context).

Also is there a convention for custom property names in CMake? You're using "EXTENSIONS" as a "magic" keyword here, I would use a "LLVM_" prefix (both to make it clear that this is a LLVM thing, and to avoid possible collision).
Since this is used for target linking, maybe make it part of the name as well? Something like "LLVM_EXTENSIONS_LINK"?

811 ↗(On Diff #198588)

If the target has to exist here and since you're using this to make it link a library, why are you going through properties in the first place? Why can't you do here: `target_link_libraries(${tool} PRIVATE ${name})

tools/polly/CMakeLists.txt
212 ↗(On Diff #198588)

That looks like leftover experiments?

@Meinersbur, I've tested and reviewed this patch, I'm curious about Polly's team opinion on the changes in current state?

I will have to try out the patch, e.g. on Windows. Please ping me if it takes to long.

cmake/modules/AddLLVM.cmake
808 ↗(On Diff #198588)

According to a recent mailing list posting, lld might need to have such tools linked-in as well for LTO.

tools/polly/CMakeLists.txt
212 ↗(On Diff #198588)

This was likely added as one of my comments: Polly-ACC requires the NVPTX target to be present in the executable it is linked to. bugpoint by default does not depend on LLVMTarget, i.e. does not contain the NVPTX target.

mehdi_amini marked an inline comment as done.May 9 2019, 8:45 PM
mehdi_amini added inline comments.
tools/polly/CMakeLists.txt
212 ↗(On Diff #198588)

Yes, I saw the code removed in CMakeLists.txt after writing this comment but forgot to delete it before sending, sorry.

serge-sans-paille marked 7 inline comments as done.May 10 2019, 1:20 AM

I will have to try out the patch, e.g. on Windows. Please ping me if it takes to long.

Thanks @Meinersbur

CMakeLists.txt
1124 ↗(On Diff #198588)

I was thinking « X Macro » :-) https://en.wikipedia.org/wiki/X_Macro

1125 ↗(On Diff #198588)

Fixed in register_llvm_extension

cmake/modules/AddLLVM.cmake
811 ↗(On Diff #198588)

Because it's illegal in cmake to call target_link_libraries in a different file, see https://cmake.org/cmake/help/v3.0/command/target_link_libraries.html (this has been changed in recent version of cmake though)

serge-sans-paille marked 3 inline comments as done.May 13 2019, 11:32 AM

@Meinersbur I'll be happy to read your feedback on windows compilation!

This didn't work for me, neither on Windows nor on Linux.

Linux error:

[138/138] Linking CXX executable bin/opt
/usr/bin/ld: cannot find -lpolly
collect2: error: ld returned 1 exit status

Windows error:

[1654/1715] Linking CXX executable bin\opt.exe
LINK : fatal error LNK1181: cannot open input file 'polly.lib'

This might indicate that "Polly" is not a recognized target during target_link_libraries. If it did, it would add lib/libPolly.a instead of -lPolly to the linker command line. My Polly is in ${LLVM_ROOT}/tools/Polly. I can see it has been picked up since ninja Polly works and "-- Registering polly as a compiler extension" is printed as well. My Linux cmake-line is:

cmake "${SRCPATH}" \
  -GNinja \
  -DCMAKE_C_COMPILER=/soft/compilers/gcc/8.2.0/linux-rhel7-x86_64/bin/gcc \
  -DCMAKE_CXX_COMPILER=/soft/compilers/gcc/8.2.0/linux-rhel7-x86_64/bin/g++ \
  -DCMAKE_INSTALL_PREFIX="${INSTALLPATH}" \
  -DCLANG_ENABLE_STATIC_ANALYZER=OFF \
  -DLLVM_PARALLEL_LINK_JOBS=2 \
  -DCLANG_ENABLE_ARCMT=OFF \
  -DCMAKE_EXPORT_COMPILE_COMMANDS=ON \
  '-DLLVM_TARGETS_TO_BUILD=X86;NVPTX' \
  -DLLVM_ENABLE_ASSERTIONS=ON \
  -DCMAKE_BUILD_TYPE=Release \
  -DPOLLY_ENABLE_GPGPU_CODEGEN=ON \
  -DLLVM_TEMPORARILY_ALLOW_OLD_TOOLCHAIN=ON \
  -DCUDA_TOOLKIT_ROOT_DIR=/soft/compilers/cuda/cuda-10.0.130/

My Extension.def contains:

//extension handlers
HANDLE_EXTENSION(polly, Polly)
#undef HANDLE_EXTENSION

@Meinersbur thanks a lot for the feedback. I was only testing with the new project layout, so it's nice you tested with the legacy layout.
I've updated the diff accordingly, it now compiles fine on my Linux box with both old-style and new-style layout.

Can you confirm it's ok on your side? I don't have access to a Windows box.

Nicola added a subscriber: Nicola.May 27 2019, 3:34 AM

Unfortunately this will stop working as soon as we switch on the new pass manager. With that, there is no canonical initialize*Passes that you can call. Do you have a plan to support the new pass manager as well?

Fail the regression tests under Linux:

$ ninja check-polly
[...]
: 'RUN: at line 1';   opt -load /home/meinersbur/build/llvm/release/lib/LLVMPolly.so -load-pass-plugin /home/meinersbur/build/llvm/release/lib/LLVMPolly.so [...]
--
Exit Code: 2

Command Output (stderr):
--
opt: CommandLine Error: Option 'polly-dependences-computeout' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options
[...]

This error typically appears when polly is statically linked into tools (LINK_POLLY_INTO_TOOLS=ON), but the shared library is loaded as well at run-time (-load /home/meinersbur/build/llvm/release/lib/LLVMPolly.so). You probably will have to adjust polly/test/lit.site.cfg.in:

if config.link_polly_into_tools == '' or \
   config.link_polly_into_tools.lower() == '0' or \
   config.link_polly_into_tools.lower() == 'n' or \
   config.link_polly_into_tools.lower() == 'no' or \
   config.link_polly_into_tools.lower() == 'off' or \
   config.link_polly_into_tools.lower() == 'false' or \
   config.link_polly_into_tools.lower() == 'notfound' or \
   config.link_polly_into_tools.lower() == 'link_polly_into_tools-notfound':

To these long turnarounds, you could test these configurations yourself. Some time ago I made an overview on which combinations of dynamic/static linking should work: https://groups.google.com/d/msg/polly-dev/vxumPMhrSEs/E50kboqlAQAJ

For LTO, the llvm_extensions might need to be linked into lld as well. The recent polly-dev thread: https://groups.google.com/d/msg/polly-dev/BeC_v_oJIVM/LAA7vVT9AwAJ

However, since we don't even have this for Polly, I would leave it up to you whether to include this in this patch review.

@Meinersbur thanks a lot for the previous review! lld integration, as well as new PM integration, will be for another commit, in order to prevent this review from growing too large

Herald added a project: Restricted Project. · View Herald TranscriptMon, Jun 3, 2:00 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I fear you'll need to plan at least for new pm right now. Otherwise your change will be obsolete in half a year. What is it really that LINK_X_INTO_TOOLS is supposed to do? Effectively bake in something that's consumable by -load or -load-plugin, right? So can we use that interface directly? Effectively, can we have statically built-in plugins, that prepopulate the plug-in loader mechanisms?

If the patch becomes too large then it should be split into separate functional units, e.g., one adding the cmake functionality, one updating the plugins and the tools.

You could also ask @beanz for a review of the cmake change.

Meinersbur requested changes to this revision.Mon, Jun 3, 3:09 PM

It still fails with the same error. LINK_POLLY_INTO_TOOLS is only set after the polly subdirectory is processed. Because it is an option, the ON value will be stored in the CMakeCache.txt and be available during the next run, it actually works after running the cmake configure step a second time. We should not expect users to do so. Because of this, any option(...) or set(... CACHED) should be done at the beginning of the file.

llvm/CMakeLists.txt
449

Note that there is LLVM_POLLY_LINK_INTO_TOOLS and LINK_POLLY_INTO_TOOLS. The former is the user-configurable option, the latter is for internal querying. At least, this is what is was meant for.

903

Polly is included here. LINK_POLLY_INTO_TOOLS will not be visible in there if set only later.

llvm/cmake/modules/AddLLVM.cmake
825

[remark] Use LLVM_ prefix for LLVM-level options.

This revision now requires changes to proceed.Mon, Jun 3, 3:09 PM
beanz added a comment.Mon, Jun 3, 5:24 PM

I have a thought for how we can make the CMake interface for this better. Instead of LLVM_EXTENSIONS being a global property if it were a pre-defined list like LLVM_ALL_PROJECTS, then the LLVM_LINK_<X>_INTO_TOOLS variables can be generated by LLVM's configuration instead of relying on Polly's to run. If you do that LLVM_LINK_POLLY_INTO_TOOLS could be set to On even if Polly isn't enabled to build, but that actually will be fine if you change the calls to target_link_libraries in the tools to use a generator expression something like: $<$<TARGET_EXISTS:Polly>,Polly>.

These changes should eliminate the order-dependence on the tools and allow the first configuration run to be accurate.

  • reorder registration to make sure options are correctly taken into account
  • make LINK_POLLY_INTO_TOOLS obsolete in favor of just using LLVM_LINK_POLLY_INTO_TOOLS

@philip.pfaffe : NewPM integeration actually works thanks to `Register${Name}Passes(llvm::PassBuilder &PB)`. It's already used by Polly for NewPM integration and we mimic that.

@beanz turns out the depdendency problem was just a matter of moving the register_llvm_extension call at the top of the file.

Take into account @philip.pfaffe remarks and improve support for new PM.

  • harmonize static/dynamic loading, although two functions are still needed, one for dynamic loading and one for static loading (new PM)
  • do not clutter the required API with extra namespaces

I've updated https://sergesanspaille.fedorapeople.org/bye.tgz to showcase a minimal example.

  • make it possible for client code to speicify the registration function for new PM, thanks @philip.pfaffe for the hint. @Meinersbur this should be ok now.
  • get Rid of the constraint on initializePasses : it can be registered suing a static constructor, no need to clutter the API with it

This fails with Polly/Linux regression tests:

********************
FAIL: Polly :: Simplify/gemm.ll (1148 of 1149)
******************** TEST 'Polly :: Simplify/gemm.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   opt  -polly-process-unprofitable  -polly-remarks-minimal  -polly-use-llvm-names  -polly-import-jscop-dir=/home/meinersbur/src/llvm/tools/polly/test/Simplify  -polly-codegen-verify  -polly-import-jscop    -polly-import-jscop-postfix=transformed -polly-simplify -analyze < /home/meinersbur/src/llvm/tools/polly/test/Simplify/gemm.ll    | FileCheck /home/meinersbur/src/llvm/tools/polly/test/Simplify/gemm.ll
--
Exit Code: 2

Command Output (stderr):
--
opt: for the   -o option: may not occur within a group!
opt: Unknown command line argument '-polly-import-jscop'.  Try: 'opt --help'
opt: Did you mean '  -o'?
opt: for the   -p option: may only occur zero or one times!
opt: for the   -o option: may not occur within a group!
opt: Unknown command line argument '-polly-simplify'.  Try: 'opt --help'
opt: Did you mean '  -o'?
FileCheck error: '-' is empty.
FileCheck command line:  FileCheck /home/meinersbur/src/llvm/tools/polly/test/Simplify/gemm.ll

I think this means that the Polly passes have not been registered (initializePollyPasses must be called someway in opt/clang_cc1/bugpoint). Linking from static libraries will NOT include Polly.o (and run its static initializers) unless it is needed to resolve at least one symbol.

PLEASE run make/ninja check-polly before uploading a patch.

llvm/tools/opt/opt.cpp
534

Where is the equivalent for this in your change? I see it's been done for clang_cc1, but not for opt/bugpoint.

polly/CMakeLists.txt
210 ↗(On Diff #203086)

[nit] Whitespace change

polly/lib/CMakeLists.txt
27

Why this change? Polly.cpp should only be necessary for the loadable module, but not for LLVM_LINK_POLLY_INTO_TOOLS.

Meinersbur requested changes to this revision.Wed, Jun 5, 10:02 AM
This revision now requires changes to proceed.Wed, Jun 5, 10:02 AM

Sorry, you might have tested it with LLVM_LINK_POLLY_INTO_TOOLS=OFF and/or BUILD_SHARED_LIBS/LLVM_LINK_LLVM_DYLIV where it might work, but unfortunately not with the default configuration using static component libraries.

beanz added inline comments.Wed, Jun 5, 10:25 AM
llvm/cmake/modules/AddLLVM.cmake
837

I don't think we should hard code this list. It would be much better if we add an option to llvm_add_executable so that extension targets denote themselves.

839

if(TARGET ...) is order dependent. That's why you need to change tools/CMakeLists.txt, which you won't need to do if you change this to work how I suggested in my earlier comment.

@Meinersbur Itested with the following configuration

cmake3 ../llvm -DLLVM_EXTERNAL_PROJECTS=bye -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=Release  -DLLVM_TARGETS_TO_BUILD=X86

cmake3 ../llvm -DLLVM_EXTERNAL_PROJECTS=bye -DBUILD_SHARED_LIBS=ON -DCMAKE_BUILD_TYPE=Release  -DLLVM_TARGETS_TO_BUILD=X86 -DLLVM_POLLY_LINK_INTO_TOOLS=OFF

with old and mono repo layout. I'll test with BUILD_SHARED_LIBS=OFF too.

Just an idea: We could avoid the explicit calls to 'initializeXYZPass' in opt/bugpoint/clang by adding Polly.cpp as a source file or object library to the executables. This would guarantee that its static initializer is called without dynamic library.

Just an idea: We could avoid the explicit calls to 'initializeXYZPass' in opt/bugpoint/clang by adding Polly.cpp as a source file or object library to the executables. This would guarantee that its static initializer is called without dynamic library.

That's what I tried to do when always adding Polly.cpp to PollyCore, but it doesn't seem to be enough, I still need to invstigate why (it is actually enough when using BUILD_SHARED_LIBS=ON)

Using @beanz idea, provide an option to add_llvm_executable to declare it as receiving plugins, which provides lower coupling with actual plugins

Tested with four configurations: BUILD_SHARED_LIBS ON/OFF and with llvm-project / legacy configuration, but *not* on Windows

As suggested by @Meinersbur, use static constructor for initialization step with legacy PM, for both shared and static libraries.

The patch looks much less intrusive now, thanks a lot to all reviewers for the provided guidance.

serge-sans-paille marked 4 inline comments as done.Sat, Jun 8, 9:00 AM
beanz added inline comments.Mon, Jun 10, 10:40 AM
llvm/cmake/modules/AddLLVM.cmake
814

Change this to llvm-plugins to match our convention and wrap it in if (NOT TARGET...) so it doesn't error if AddLLVM is included twice.

Meinersbur requested changes to this revision.Mon, Jun 10, 12:26 PM

That's what I tried to do when always adding Polly.cpp to PollyCore, but it doesn't seem to be enough, I still need to invstigate why (it is actually enough when using BUILD_SHARED_LIBS=ON)

With static libraries, you had the following structure:

PollyCore.a:
  RegisterPasses.o
  Polly.o <-- static initializer here, but never used

LLVMPolly.so (for use with -load mechanism)
  RegisterPasses.o
  Polly.o <-- static initializer here, executed when loading the .so

opt:
  main.o <-- call to initializePollyPass removed

With static linking, there is no reason for the linker to add Polly.o to the opt executable because no symbol from Polly.o (or any of PollyCore) was required to resolve any symbol in opt. Building a shared library using the object library files will include all object files, since it does not know which symbols will be used at runtime. I recommend reading http://www.lurklurk.org/linkers/linkers.html#staticlibs and https://www.bfilipek.com/2018/02/static-vars-static-lib.html.

What I proposed was

PollyCore.a:
  RegisterPasses.o

LLVMPolly.so (for use with -load mechanism)
  RegisterPasses.o
  Polly.o

opt: // or any ENABLE_PLUGINS tool
  main.o
  Polly.o // via add_executable(opt Polly.cpp) or target_sources

Adding the object file explicitly to the add_executable will add it unconditionally, even if no none of its symbol is required, like for LLVMPolly.so.

In the latest update you changed the location of the static initalizer to RegisterPasses.o, which contains the symbol llvmGetPassPluginInfo which is pulled-in by the new pass manager plugin system. This makes an unfortunate dependence of the static initializer on the llvmGetPassPluginInfo mechanism (which I think only works for opt in the current patch).

There is a reason why pass loading in Polly is organized as it is.

llvm/cmake/modules/AddLLVM.cmake
814

[serious] I get multiple of these errors by cmake:

CMake Error at cmake/modules/AddLLVM.cmake:812 (add_custom_target):
  add_custom_target cannot create target "LLVM_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/unittests/CMakeLists.txt:2 (include)
llvm/tools/opt/NewPMDriver.cpp
292

[serious] This will pull-in the symbol llvmGetPassPluginInfo from the plugin for opt, but what about the other tools (bugpoint/clang)?

polly/lib/Support/RegisterPasses.cpp
727

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

This revision now requires changes to proceed.Mon, Jun 10, 12:26 PM
serge-sans-paille marked 3 inline comments as done.

@beanz : In addition to your suggested changes, I've renamed the cmake utility into add_llvm_pass_plugin to match other functions from AddLLVM.cmake

@Meinersbur : thanks a lot for the polly layout explanation. I've slighlty updated your suggection like this:

PollyCore.a:
  RegisterPasses.o
  polly_static_entry_point.cpp

LLVMPolly.so (for use with -load mechanism)
  RegisterPasses.o
  polly_plugin_entry_point.cpp

initialization is done in both cases through a static initializer in RegisterPasses.cpp. llvmGetPassPluginInfo is only available in polly_plugin_entry_point.cpp.

polly/lib/Support/RegisterPasses.cpp
727

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.

Mostly documentation update + helper function renaming.

@Meinersbur, I've tested the setup with static and dynamic builds on Linux, please let me know your thoughts on this.

Meinersbur added inline comments.Wed, Jun 26, 5:30 AM
llvm/cmake/modules/AddLLVM.cmake
810

[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

853

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

llvm/docs/WritingAnLLVMPass.rst
1222

"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

llvm/tools/CMakeLists.txt
45

What is the reason to have this after add_llvm_implicit_projects in contrast to the other LLVM subprojects?

polly/lib/Support/RegisterPasses.cpp
727

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?

serge-sans-paille marked an inline comment as done.

@Meinersbur your comment and my devs crossed, but this should be fine. This update enables new PM static plugin support for clang, something that was lacking to polly previously. I've tested it a bit and it builds fine with static setup, still need to test it with more config.

serge-sans-paille marked 10 inline comments as done.Wed, Jun 26, 6:37 AM
serge-sans-paille added inline comments.
llvm/cmake/modules/AddLLVM.cmake
810

I'm no longer using that mechanism, it was showing some limits wrt. cmake generator-expression and export set.

853

I'm no longer using target_sources, it should be fine now.

llvm/docs/WritingAnLLVMPass.rst
1222

I'm not super happy with the new title, but I gave it a try.

llvm/tools/CMakeLists.txt
45

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.

polly/lib/Support/RegisterPasses.cpp
727

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)