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

[serge-sans-paille]

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.

[serge-sans-paille]

Added a Bye project in examples/
Fixed linking of plugins into core tools
Fixed dependency issue

[Meinersbur]

Thank you for adding the Bye pass. It is really useful! Is there a specific reason to not modify the Hello pass?

---

If I enable both passes statically (LLVM_BYE_LINK_INTO_TOOLS=ON and LLVM_POLLY_LINK_INTO_TOOLS=ON), the following regression tests fail (ninja check-llvm):

Failing Tests (8):
    LLVM :: DebugInfo/debugify-each.ll
    LLVM :: Other/new-pm-defaults.ll
    LLVM :: Other/new-pm-thinlto-defaults.ll
    LLVM :: Other/opt-O0-pipeline.ll
    LLVM :: Other/opt-O2-pipeline.ll
    LLVM :: Other/opt-O3-pipeline.ll
    LLVM :: Other/opt-Os-pipeline.ll
    LLVM :: Transforms/LoopVectorize/X86/reg-usage.ll

The pass output such as

Bye: foo
Bye: goo
Bye: bar
Bye: hoo

seem to interfere with the CHECK lines in the test cases.

---

Using the configuration LLVM_LINK_LLVM_DYLIB=1 and LLVM_POLLY_LINK_INTO_TOOLS=ON, the following tests fail:

Failing Tests (10):
    LLVM :: BugPoint/compile-custom.ll
    LLVM :: BugPoint/crash-narrowfunctiontest.ll
    LLVM :: BugPoint/func-attrs-keyval.ll
    LLVM :: BugPoint/func-attrs.ll
    LLVM :: BugPoint/invalid-debuginfo.ll
    LLVM :: BugPoint/metadata.ll
    LLVM :: BugPoint/named-md.ll
    LLVM :: BugPoint/remove_arguments_test.ll
    LLVM :: BugPoint/replace-funcs-with-null.ll
    LLVM :: BugPoint/unsymbolized.ll

The error output is:

: CommandLine Error: Option 'disable-basicaa' registered more than once!
LLVM ERROR: inconsistency in registered CommandLine options"

---

As expected, on Windows, I get the following linker error with both LLVM_BYE_LINK_INTO_TOOLS=ON and LLVM_POLLY_LINK_INTO_TOOLS=ON:

[1/1] Linking CXX executable bin\clang.exe
FAILED: bin/clang.exe
cmd.exe /C "cd . && "C:\Program Files\CMake\bin\cmake.exe" -E vs_link_exe --intdir=tools\clang\tools\driver\CMakeFiles\clang.dir --rc=C:\PROGRA~2\WI3CF2~1\10\bin\100177~1.0\x64\rc.exe --mt=C:\PROGRA~2\WI3CF2~1\10\bin\100177~1.0\x64\mt.exe --manifests  -- C:\PROGRA~2\MICROS~1\2017\COMMUN~1\VC\Tools\MSVC\1416~1.270\bin\Hostx64\x64\link.exe /nologo @CMakeFiles\clang.rsp  /out:bin\clang.exe /implib:lib\clang.lib /pdb:bin\clang.pdb /version:0.0  /machine:x64 /STACK:10000000 /INCREMENTAL:NO /subsystem:console  && cmd.exe /C "cd /D C:\Users\meinersbur\build\llvm\release\tools\clang\tools\driver && "C:\Program Files\CMake\bin\cmake.exe" -E copy C:/Users/meinersbur/build/llvm/release/bin/clang.exe C:/Users/meinersbur/build/llvm/release/./bin/clang++.exe && cd /D C:\Users\meinersbur\build\llvm\release\tools\clang\tools\driver && "C:\Program Files\CMake\bin\cmake.exe" -E copy C:/Users/meinersbur/build/llvm/release/bin/clang.exe C:/Users/meinersbur/build/llvm/release/./bin/clang-cl.exe && cd /D C:\Users\meinersbur\build\llvm\release\tools\clang\tools\driver && "C:\Program Files\CMake\bin\cmake.exe" -E copy C:/Users/meinersbur/build/llvm/release/bin/clang.exe C:/Users/meinersbur/build/llvm/release/./bin/clang-cpp.exe""
LINK: command "C:\PROGRA~2\MICROS~1\2017\COMMUN~1\VC\Tools\MSVC\1416~1.270\bin\Hostx64\x64\link.exe /nologo @CMakeFiles\clang.rsp /out:bin\clang.exe /implib:lib\clang.lib /pdb:bin\clang.pdb /version:0.0 /machine:x64 /STACK:10000000 /INCREMENTAL:NO /subsystem:console /MANIFEST /MANIFESTFILE:bin\clang.exe.manifest" failed (exit code 1169) with the following output:
Bye.lib(Bye.cpp.obj) : error LNK2005: llvmGetPassPluginInfo already defined in Polly.lib(RegisterPasses.cpp.obj)
bin\clang.exe : fatal error LNK1169: one or more multiply defined symbols found
ninja: build stopped: subcommand failed.

llvm/cmake/modules/AddLLVM.cmake

888–890

[nit] Please remove commented code entirely

llvm/examples/Bye/Bye.cpp

15

Could you add a test that verifies that the Bye test has been loaded and is working?

[serge-sans-paille]

• Test extension point registration when extension is built-in
• Correctly handle -DLLVM_LINK_LLVM_DYLIB=0
• Fix a few details

[serge-sans-paille]

@Meinersbur I've updated the test case to test extension point if the extension is linked in, this is not so intrusive, I'm happy with the solution.

Also fixed some linkage options when using llvm dynlib, validation looks nice on my side.

[serge-sans-paille]

Make validation more resilient depending on shared/static build.

[Meinersbur]

Did you resolve the conflicting llvmGetPassPluginInfo symbols for windows?

[serge-sans-paille]

@Meinersbur nop, forgot that one, I'll have a look, thanks for pointing that out.

[serge-sans-paille]

Also fix symbol redefinition during static builds on Windows

[serge-sans-paille]

@Meinersbur up!

[Meinersbur]

Sorry for the break.
Unfortunately the patch does not apply cleanly anymore. Can you rebase to latest trunk?

I surprisingly did not know about __declspec(selectany). I still think having the symbol only defined in that loadable module, but not in the statically linked library is the cleaner solution than relying on linker cleverness. If unused by design, there is no reason why the symbol should be even there. Can you convince me otherwise?

llvm/include/llvm/Support/Compiler.h

162 ↗

(On Diff #213176)

With __declspec(selectany), the FIXME should be removed.

However, I think this does not exactly match __attribute__((__weak__)): Weak symbols can still be undefined whereas selectany, if I understand MS's docs correctly, requires one valid symbol during the linking phase (Well, we currently don't support LLVM dlls anyway). This might be what you want for this patch, but classically I'd expect a weak symbol as a symbol that can resolve to NULL.

Also, I am not sure mingw/cygwin support it (I'd try out once I can apply the patch again).

[serge-sans-paille]

@Meinersbur patch rebased. I removed the linker trick (which only work for global variables, not function, anyway), as it's no longer needed:

target_compile_definitions(${name} PRIVATE LLVM_${name_upper}_LINK_INTO_TOOLS)

is used to only provide the symbol if we're in shared library mode.

[serge-sans-paille]

Up :-)

[serge-sans-paille]

@Meinersbur any feedback on this update?

[andwar]

llvm/examples/Bye/Bye.cpp

54

[nit] Empty line

llvm/examples/Bye/CMakeLists.txt

10

Are all these libraries required? I tried building on Darwin and for me LLVMSupport, LLVMCore and LLVMipo were sufficient.

29

[nit] Empty line

[serge-sans-paille]

Updates:

• fix typo in documentation
• take into account @andwar advices
• improve shared/static build automation

[Meinersbur]

llvm/test/Feature/load_extension.ll

2

It would be preferable to have a "REQUIRES" line that checks that the Bye pass has been linked-in dynamically.

Alternatively, use the lit.cfg to expend to the required -load= argument if required, so this test checks with and without LLVM_BYE_LOAD_INTO_TOOLS

3

[serious] Use FileCheck

llvm/test/lit.cfg.py

200–203

[serious] This kind of shell expansion does not work on Windows:

$ "c:\users\meinersbur\build\llvm\release\bin\filecheck.exe" "C:\Users\meinersbur\src\llvm\test\Other\new-pm-thinlto-defaults.ll" "--check-prefixes=CHECK-O,CHECK-O1,CHECK-POSTLINK-O,${LLVM_CHECK_EXT},CHECK-POSTLINK-O1"
# command stderr:
Supplied check-prefix is invalid! Prefixes must be unique and start with a letter and contain only alphanumeric characters, hyphens and underscores

error: command failed with exit status: 2

Polly 'solves' this by adding itself to the pass manager only when another command line flag is added (-polly) which would be less intrusive.

llvm/test/lit.site.cfg.py.in

53

See https://cmake.org/cmake/help/latest/command/if.html for which values cmake considers true-ish.

polly/lib/Support/RegisterPasses.cpp

727

[serious] LLVM_POLLY_LINK_INTO_TOOLS is a cmake configuration parameter, but not a preprocessor symbol. Hence, LLVM_POLLY_LINK_INTO_TOOLS is never defined.

Error on Windows:

Bye.lib(Bye.cpp.obj) : error LNK2005: llvmGetPassPluginInfo already defined in Polly.lib(RegisterPasses.cpp.obj)
bin\opt.exe : fatal error LNK1169: one or more multiply defined symbols found

[Meinersbur]

Keep in mind that for static linking you will need something that pulls-in a symbol from the pass static library. In this patch, NewPMDriver.cpp does it for opt by calling get##Ext##PluginInfo(). In clang, it is BackendUtil.cpp. But nothing for bugpoint hence it doesn't contain either Polly not the Goodbye pass (However, llvm-reduce is in the works, we might consider bugpoint deprecated).

Could you add documentation for how to write a tool that uses loadable plugins to document the way it should be done?

[Meinersbur]

polly/lib/Support/RegisterPasses.cpp

727

Before you try to fix this preprocessor symbol, consider that Polly compiles a loadable module (to be used with -load) and a library (static or dynamic depending on BUILD_SHARED_LIBS) independent of LLVM_POLLY_LINK_INTO_TOOLS. That is, the loadable module must contain llvmGetPassPluginInfo, but not the library. That is, a preprocessor symbol that is the same for both will not work.

PLEASE, just keep the Polly.cpp (and move llvmGetPassPluginInfo in there; the static initializer indeed has to stay here as it will be used by both), it will make things easier as it already has been shown to work already. Do the same for Bye.

If you really insist on removing the Polly.cpp, do so in a follow-up patch. In that case you will have to rework the CMakeLists.txt to only build one, either the loadable module or the library.

[serge-sans-paille]

First rework pass, sevral comments have been taken into account but not all of them.

[serge-sans-paille]

Take into account most of the reviews.

[serge-sans-paille]

In D61446#1681841, @Meinersbur wrote:

Keep in mind that for static linking you will need something that pulls-in a symbol from the pass static library. In this patch, NewPMDriver.cpp does it for opt by calling get##Ext##PluginInfo(). In clang, it is BackendUtil.cpp. But nothing for bugpoint hence it doesn't contain either Polly not the Goodbye pass (However, llvm-reduce is in the works, we might consider bugpoint deprecated).

Done, thanks.

Could you add documentation for how to write a tool that uses loadable plugins to document the way it should be done?

It's natural if we use the NewPM, so I've added some doc in that direction.

polly/lib/Support/RegisterPasses.cpp

727

In add_llvm_pass_plugin there's

if(LLVM_${name_upper}_LINK_INTO_TOOLS)
    target_compile_definitions(${name} PRIVATE LLVM_${name_upper}_LINK_INTO_TOOLS)
    set_property(TARGET ${name} APPEND PROPERTY COMPILE_DEFINITIONS LLVM_LINK_INTO_TOOLS)
    if (TARGET intrinsics_gen)
      add_dependencies(obj.${name} intrinsics_gen)
    endif()
endif()

So the macro definition should be there.

[serge-sans-paille]

@Meinersbur to make your reviewer job easier, I've setup validation for that patch, see https://github.com/serge-sans-paille/llvm-project/pull/2/checks
It build and validates fine.

[Meinersbur]

I still have to try out the patch.

In D61446#1756211, @serge-sans-paille wrote:

@Meinersbur to make your reviewer job easier, I've setup validation for that patch, see https://github.com/serge-sans-paille/llvm-project/pull/2/checks
It build and validates fine.

Thanks!

[suggestion] Also add -DPOLLY_ENABLE_GPGPU_CODEGEN=ON and -DLLVM_ENABLE_ASSERTIONS=ON, and run ninja check-polly. It currently does not run any polly code, just compiles it.

llvm/docs/WritingAnLLVMPass.rst

1200

[typo] when sets to

llvm/test/Feature/load_extension.ll

2–3

[suggestion] The established pattern is to pipe the output to FileCheck without using a temporary file.

llvm/tools/bugpoint/bugpoint.cpp

232–234

Nice!

polly/include/polly/RegisterPasses.h

27

[style] Start function names with lower case letters

polly/lib/Plugin/Polly.cpp

18–31

[serious] Since opt, ... etc. don't call initializePollyPasses directly anymore, I think the static initializer has to moved to RegisterPasses.cpp.

[serge-sans-paille]

Handle all @Meinersbur comment, rebuild on github with extra flags pending.

[Alouest]

Greetings,

I stumbled upon a bug while toying with your example Bye.cpp. I modified your pass to make a ModulePass and while it works with

/build/bin/opt -goodbye -wave-goodbye test.ll

It's making clang crash with the command

/build/bin/clang test.c

I've included the pass, the error logs and some files to reproduce the bug (hello2.bc is supposed to be placed in the directory where you start your code).
It seems that the bug is coming from StringMap.cpp, it looks like NumBuckets is not always initialized when accessed, raising a SegFault.
In my example, this function is called by Module::getFunction (StringRef).

I'm a beginner in LLVM so I might just be doing dumb things.

test-df7efc.c151 KBDownload

Bye.cpp5 KBDownload

test.c125 BDownload

test-df7efc.sh1 KBDownload

hello2.bc4 KBDownload

test.ll2 KBDownload

log6 KBDownload

[serge-sans-paille]

@Alouest thanks for the reproducer. I confirm your issue, and also confirm that if you pass -fexperimental-new-pass-manager -O1 to clang (i.e. use the new pass manager) it works fine.

The registration point you're using for the legacy passmanager is specific to Function passes, the one you should be using is EP_ModuleOptimizerEarly

static llvm::RegisterStandardPasses RegisterBye(
    llvm::PassManagerBuilder::EP_ModuleOptimizerEarly,
    [](const llvm::PassManagerBuilder &Builder,
       llvm::legacy::PassManagerBase &PM) {
    PM.add(new LegacyBye());
    });

[serge-sans-paille]

@Meinersbur I have a strange symbol issue when activating the GPu part: https://github.com/serge-sans-paille/llvm-project/pull/2/checks?check_run_id=324901896#step:5:4888
Any hint?

[Meinersbur]

In D61446#1763278, @serge-sans-paille wrote:

@Meinersbur I have a strange symbol issue when activating the GPu part: https://github.com/serge-sans-paille/llvm-project/pull/2/checks?check_run_id=324901896#step:5:4888
Any hint?

I mentioned this already here: https://groups.google.com/d/msg/polly-dev/vxumPMhrSEs/uE7OfPojCwAJ

Polly uses llvm::Linker, which is not used in opt and hence thrown out when statically linking the opt executable. Try -DLLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON.

Note this never worked before, so you don't need to bother for this patch. It's what I think is a flaw in the plugin system with statically linked executables.

[serge-sans-paille]

@Meinersbur : with this version, validation passes using static build on three platforms, with the following config:

-DLLVM_ENABLE_PROJECTS='polly;clang' -DPOLLY_ENABLE_GPGPU_CODEGEN=ON  -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_POLLY_LINK_INTO_TOOLS=ON

and also on dynamic builds, with the following config, except on windows:

-DLLVM_ENABLE_PROJECTS='polly;clang' -DPOLLY_ENABLE_GPGPU_CODEGEN=OFF -DLLVM_ENABLE_ASSERTIONS=ON -DLLVM_POLLY_LINK_INTO_TOOLS=OFF

Is there any other setup you'd like to be added to the github action validation?

[Meinersbur]

I tested the configurations mentioned at https://groups.google.com/d/topic/polly-dev/vxumPMhrSEs/discussion again and did not find any issues that were not there before. Therefore IMHO this is good to go.

[Meinersbur]

Btw, this patch did not apply cleanly on trunk. I instead test the version from your repository.

[serge-sans-paille]

Patch rebased, validation ongoing. I'll merge that once I'm back from holidays, don't feel like commiting that 3h before holidays.

Thanks a lot @Meinersbur for all the reviews!

[thakis]

llvm/test/lit.site.cfg.py.in

6

FYI, this is a now-discouraged pattern. It's better to use llvm_canonicalize_cmake_booleans in the cmake file instead. See https://reviews.llvm.org/D56912 for an example.

[serge-sans-paille]

This comment has been deleted.

llvm/test/lit.site.cfg.py.in

6

Sure! Thanks for pointing at this, I'll fix that once the builds are back to normal.

[sdmitriev]

llvm/test/Other/opt-O2-pipeline.ll

1

Looks like there is a redundant '-' at the end of the check prefix that need to be removed (CHECK- => CHECK). And opt-O3-pipeline.ll and opt-Os-pipeline.ll tests also have this issue.

[fhahn]

There seems to be a problem with this patch on macOS. I've XFAIL'd it in rGdb82fc5dd80ff14798e7f1c35dd7e593f6409ba3. This should unblock the macOS builders, but please take a look at the problem.

[svenvh]

This change seems to be causing a problem with the nightly packages from apt.llvm.org.

CMake Error at /usr/lib/llvm-10/lib/cmake/llvm/LLVMExports.cmake:1357 (message):

  The imported target "Bye" references the file

     "/usr/lib/llvm-10/lib/libBye.a"

  but this file does not exist.  Possible reasons include:

  * The file was deleted, renamed, or moved to another location.

  * An install or uninstall procedure did not complete successfully.

  * The installation package was faulty and contained

     "/usr/lib/llvm-10/lib/cmake/llvm/LLVMExports.cmake"

  but not all the files it references.

Call Stack (most recent call first):

  /usr/lib/llvm-10/cmake/LLVMConfig.cmake:260 (include)

  CMakeLists.txt:34 (find_package)

-- Configuring incomplete, errors occurred!

For reference: we saw a similar issue with D69416.

[serge-sans-paille]

In D61446#1805413, @svenvh wrote:

This change seems to be causing a problem with the nightly packages from apt.llvm.org.

https://reviews.llvm.org/D72255 should do the trick (?)

[efriedma]

http://lab.llvm.org:8011/builders/aosp-O3-polly-before-vectorizer-unprofitable is currently broken, I think due to this patch. It looks like polly isn't getting linked into clang by default anymore?