[Opt] Enable statically-linked plugin support #79227

wsmoses · 2024-01-23T23:38:42Z

Companion PR to #70171 except now for opt rather than clang.

This PR enables using custom passes in plugins, when statically linked against opt.

See #79205 for a standalone bazel PR needed for this to be used in a different build system.

efriedma-quic · 2024-01-24T00:15:31Z

Let me see if I understand your constraints correctly:

You want to add a plugin to an "unmodified" opt tool.
"Unmodified" means you can't modify any source files or build settings.
This is currently impossible: we don't install the relevant object files, so there's no way to refer to them. But [CMake/Bazel] Support usage of opt driver as a library #79205 hacks the build config to install the relevant object files as a library, so you can build your own binary.

If making the "opt" tool a library is actually helpful, I guess I'm not really against it... but please add a proper entry-point instead of abusing dlsym().

wsmoses · 2024-01-24T00:21:22Z

What would you propose in the alternate? I can only otherwise think of adding a global variable, like in the earliest revisions of #70171 .

efriedma-quic · 2024-01-24T00:57:17Z

Rename main() to OptDriverMain(), add a new file opt-executable.cpp that's a wrapper around it. Add whatever parameters you need to OptDriverMain().

Alternatively, adding a global variable to opt.cpp isn't that terrible. We want to avoid global variables in core libraries so we don't have issues with shared state, but I wouldn't expect opt-driver to be used in that sort of context.

github-actions · 2024-01-24T20:22:13Z

✅ With the latest revision this PR passed the C/C++ code formatter.

wsmoses · 2024-01-25T02:07:41Z

@efriedma-quic this now does the new file with wrapper, with tests, as discussed. Mind reviewing?

aeubanks · 2024-01-25T18:34:35Z

if this is just to be used in non-core tools, I'd prefer a llvm::Registry approach (basically the global variable approach @efriedma-quic mentioned), like how clang extensions work (e.g. here). then around here we can invoke all of those

that seems much simpler than this PR + #79205

efriedma-quic · 2024-01-25T20:25:36Z

that seems much simpler than this PR + #79205

I'm not sure I understand what you're getting at with the reference to #79205 . Most of the decisions there were constrained by producing something that actually builds correctly in all configurations. The goal is to allow users to build an "opt-with-a-plugin" without rebuilding LLVM. So we put the logic of opt into a library. We have to be able to build that library as a shared library for -DBUILD_SHARED_LIBS builds, so that library can't contain the definition of main(). So in sum, we need to have a library that contains all the logic, and exposes optMain() to call that logic.

We could remove the parameter from optMain, and use a global variable instead, but that doesn't really simplify anything.

aeubanks · 2024-01-25T20:44:43Z

I vaguely remember the end result of the clang patch being that internally we cloned the build target for clang but with an extra dependency on a file that registered a clang plugin via a static constructor. If we did the same thing here, we wouldn't need these patches. I may be misremembering though. @wsmoses am I misunderstanding?

efriedma-quic · 2024-01-25T20:51:38Z

The clang version of this only supported Bazel builds, not CMake builds, so it didn't have to worry about -DBUILD_SHARED_LIBS. (I didn't realize this at the time because the patch didn't actually touch those bits.)

wsmoses · 2024-01-27T02:44:32Z

So the way the clang one worked, was as follows:

Bazel's build for clang already separated all of the clang driver libraries into a separate clang-driver library (

llvm-project/utils/bazel/llvm-project-overlay/clang/BUILD.bazel

Line 2307 in ff05c30

cc_library(

) which then to build clang in Bazel one just makes an executable with it (

llvm-project/utils/bazel/llvm-project-overlay/clang/BUILD.bazel

Line 2298 in ff05c30

expand_template(

).

For our custom clang, we simply linked the clang-driver against whatever plugin we wanted.

Doing the same for opt has two issues:

There is no opt-driver like library (instead the bazel executable contains all of the functionality).
Opt plugins are only loaded by passing in a path to a .so to load, and do not have an alternate registration mechanism (e.g. registry, list of args, other global, etc).

Originally the first Bazel PR (#79205) was a 2-line bazel patch which added a similar opt-driver library target in Bazel, like there exists already for clang. Code review requested that if this was going to happen, one should also make a library for the opt driver in CMake as well.

Unfortunately making such a library in CMake is a non-trivial task, if one wants to permit all build targets (e.g. shared libs, etc), which obviously we need to support. Specifically, it required creating a separate opt.cpp file which just contains main, as distinct from the actual opt functionality. This was required to make shared library linkage work (among other minor things, like not linking the new lib into libLLVM, etc).

Since the goal was to support additional plugins, we went ahead and just added the vector of callbacks to the optMain function in the new library.

We could also support a registry, and remove the vector of callbacks to optMain. However, the need for the separate opt.cpp with just main for each executable variant [e.g. actual opt, this test plugin opt], is required for CMake + build variants.

In light of that I'd say the two most reasonable design decisions are to either:

pass the plugins as a vector to optMain, since we already have to have an optMain for the cmake library
add a registry, but remove support for CMake and only allow it for Bazel.

--

is nice because all LLVM users can do something like this. Also incidentally this is somewhat similar to how this works for mlir with mlirOptMain (

llvm-project/mlir/lib/Tools/mlir-opt/MlirOptMain.cpp

Line 504 in f826f55

LogicalResult mlir::MlirOptMain(llvm::raw_ostream &outputStream,

). However, this is different from clang, and each downstream user needs to have their own clang
is code-wise simpler, but only works for Bazel.

aeubanks

thanks for the explanation

I took a look at the llvm driver code and it seems pretty similar to this, so I'm ok with this now

perhaps this might conflict with the llvm driver stuff if we ever decide to add opt to it, but we can cross that bridge when we get there (e.g. expose a global variable in opt-driver)

aeubanks · 2024-01-30T21:43:40Z

llvm/test/Feature/load_staticextension.ll

+; REQUIRES: x86-registered-target
+; RUN: opt-printplugin %s -passes="printpass" -disable-output 2>&1 | FileCheck %s
+
+; REQUIRES: plugins


I don't think this actually requires plugins, that's only necessary for shared library plugins IIUC

aeubanks · 2024-01-30T21:44:35Z

llvm/tools/opt/test/CMakeLists.txt

@@ -0,0 +1,8 @@
+add_llvm_tool(opt-printplugin


make this add_llvm_example? then test will need REQUIRES: examples

aeubanks · 2024-01-30T21:46:05Z

llvm/tools/opt/lib/CMakeLists.txt

+  STATIC
+  NewPMDriver.cpp
+  optdriver.cpp
+  PARTIAL_SOURCES_INTENDED


don't need PARTIAL_SOURCES_INTENDED anymore

aeubanks · 2024-01-30T21:47:26Z

llvm/tools/opt/test/CMakeLists.txt

+  SUPPORT_PLUGINS
+  )
+target_link_libraries(opt-printplugin PRIVATE LLVMOptDriver)
+export_executable_symbols_for_plugins(opt-printplugin)


do we need this for an example/test binary?

aeubanks · 2024-01-30T21:47:44Z

llvm/tools/opt/test/opt-printplugin.cpp

@@ -0,0 +1,67 @@
+//===- opt-printplugin.cpp - The LLVM Modular Optimizer
+//-------------------------------===//


aeubanks · 2024-01-30T21:48:51Z

llvm/tools/opt/test/opt-printplugin.cpp

+
+} // namespace
+
+extern "C" int optMain(int argc, char **argv,


optMain() shouldn't be extern "C" since it uses C++ classes. this mirrors the llvm driver support, e.g. llvm/cmake/modules/llvm-driver-template.cpp.in

aeubanks · 2024-01-30T22:28:54Z

llvm/tools/opt/lib/CMakeLists.txt

@@ -0,0 +1,9 @@
+# We don't want to link this into libLLVM


for my understanding, how do we tell the build system not to link this into libLLVM?

aeubanks · 2024-01-30T22:30:00Z

llvm/tools/opt/CMakeLists.txt

-target_link_libraries(opt PRIVATE LLVMOptDriver)
-
-export_executable_symbols_for_plugins(opt)
+add_subdirectory(lib)


(can't comment above due to github limitations) should LLVM_LINK_COMPONENTS go down to llvm/tools/opt/lib/CMakeLists.txt?

aeubanks · 2024-01-30T22:31:34Z

llvm/tools/opt/lib/NewPMDriver.cpp

-                            "specified (JSON) file (should be abs path as we use"
-                            " append mode to insert new JSON objects)"),
-                   cl::value_desc("filename"), cl::init(""));
+cl::opt<std::string> VerifyDIPreserveExport(


can you commit the formatting changes ahead of time

wsmoses requested review from jyknight, ftynse, jdoerfert, aeubanks and efriedma-quic January 23, 2024 23:38

wsmoses mentioned this pull request Jan 24, 2024

[CMake/Bazel] Support usage of opt driver as a library #79205

Merged

wsmoses force-pushed the optplugin branch from 801e49c to 99ea48f Compare January 24, 2024 20:19

wsmoses added 2 commits January 24, 2024 21:05

[Opt] Enable support for statically linked plugins

4aaf28f

fix

6ea94cb

wsmoses force-pushed the optplugin branch from 99ea48f to 6ea94cb Compare January 25, 2024 02:06

wsmoses requested a review from nico January 25, 2024 02:08

aeubanks reviewed Jan 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Opt] Enable statically-linked plugin support #79227

[Opt] Enable statically-linked plugin support #79227

wsmoses commented Jan 23, 2024

efriedma-quic commented Jan 24, 2024

wsmoses commented Jan 24, 2024

efriedma-quic commented Jan 24, 2024

github-actions bot commented Jan 24, 2024 •

edited

Loading

wsmoses commented Jan 25, 2024

aeubanks commented Jan 25, 2024

efriedma-quic commented Jan 25, 2024

aeubanks commented Jan 25, 2024

efriedma-quic commented Jan 25, 2024

wsmoses commented Jan 27, 2024 •

edited

Loading

aeubanks left a comment

aeubanks Jan 30, 2024

aeubanks Jan 30, 2024

aeubanks Jan 30, 2024

aeubanks Jan 30, 2024

aeubanks Jan 30, 2024

aeubanks Jan 30, 2024

aeubanks Jan 30, 2024

aeubanks Jan 30, 2024

aeubanks Jan 30, 2024

		@@ -0,0 +1,67 @@
		//===- opt-printplugin.cpp - The LLVM Modular Optimizer
		//-------------------------------===//

[Opt] Enable statically-linked plugin support #79227

Are you sure you want to change the base?

[Opt] Enable statically-linked plugin support #79227

Conversation

wsmoses commented Jan 23, 2024

efriedma-quic commented Jan 24, 2024

wsmoses commented Jan 24, 2024

efriedma-quic commented Jan 24, 2024

github-actions bot commented Jan 24, 2024 • edited Loading

wsmoses commented Jan 25, 2024

aeubanks commented Jan 25, 2024

efriedma-quic commented Jan 25, 2024

aeubanks commented Jan 25, 2024

efriedma-quic commented Jan 25, 2024

wsmoses commented Jan 27, 2024 • edited Loading

aeubanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 24, 2024 •

edited

Loading

wsmoses commented Jan 27, 2024 •

edited

Loading