[PATCH] D61446: Generalize the pass registration mechanism used by Polly to any third-party tool

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur requested changes to this revision.
Meinersbur added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: llvm/cmake/modules/AddLLVM.cmake:856-858
+      #set_property(TARGET ${llvm_plugin_target} APPEND PROPERTY SOURCES $<TARGET_OBJECTS:obj.${llvm_extension}>)
+      #set_property(TARGET ${llvm_plugin_target} APPEND PROPERTY LINK_LIBRARIES $<TARGET_PROPERTY:${llvm_extension},LINK_LIBRARIES>)
+      #set_property(TARGET ${llvm_plugin_target} APPEND PROPERTY INTERFACE_LINK_LIBRARIES $<TARGET_PROPERTY:${llvm_extension},INTERFACE_LINK_LIBRARIES>)
----------------
[nit] Please remove commented code entirely


================
Comment at: llvm/examples/Bye/Bye.cpp:14
+bool runBye(Function &F) {
+  errs() << "Bye: ";
+  errs().write_escaped(F.getName()) << '\n';
----------------
Could you add a test that verifies that the `Bye` test has been loaded and is working?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61446/new/

https://reviews.llvm.org/D61446