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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur requested changes to this revision.
Meinersbur added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/test/Feature/load_extension.ll:1
+; RUN: not test -f %llvmshlibdir/libBye%shlibext || opt %s -load=%llvmshlibdir/libBye%shlibext -goodbye -wave-goodbye \
+; RUN:   -disable-output 2>&1 | grep Bye
----------------
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


================
Comment at: llvm/test/Feature/load_extension.ll:2
+; RUN: not test -f %llvmshlibdir/libBye%shlibext || opt %s -load=%llvmshlibdir/libBye%shlibext -goodbye -wave-goodbye \
+; RUN:   -disable-output 2>&1 | grep Bye
+; REQUIRES: plugins
----------------
[serious] Use FileCheck


================
Comment at: llvm/test/lit.cfg.py:193-196
+if config.linked_bye_extension:
+    llvm_config.with_environment('LLVM_CHECK_EXT', 'CHECK-EXT')
+else:
+    llvm_config.with_environment('LLVM_CHECK_EXT', 'CHECK-NOEXT')
----------------
[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.


================
Comment at: llvm/test/lit.site.cfg.py.in:49
 config.has_plugins = @LLVM_ENABLE_PLUGINS@
+config.linked_bye_extension = "@LLVM_BYE_LINK_INTO_TOOLS@".lower() in ("on", 1)
 
----------------
See https://cmake.org/cmake/help/latest/command/if.html for which values cmake considers true-ish.


================
Comment at: polly/lib/Support/RegisterPasses.cpp:726
+
+#ifndef LLVM_POLLY_LINK_INTO_TOOLS
+extern "C" LLVM_ATTRIBUTE_WEAK ::llvm::PassPluginLibraryInfo
----------------
[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
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61446