[Plugins] Add a slim plugin API to work together with the new PM
ClosedPublic

Authored by philip.pfaffe on Jul 11 2017, 8:06 AM.

Details

Summary

Add a new plugin API. This closes the gap between pass registration and out-of-tree passes for the new PassManager.

Unlike with the existing API, interaction with a plugin is always initiated from the tools perspective. I.e., when a plugin is loaded by the PluginLoader, it resolves and calls a well-known symbol llvmPluginGetInfo to obtain details about the plugin. The fundamental motivation is to get rid of as many global constructors as possible.
The API exposed by the plugin info is kept intentionally minimal.

Diff Detail

Repository
rL LLVM
philip.pfaffe created this revision.Jul 11 2017, 8:06 AM

Retain backwards compatibility with plugins for the legacy PM.

lksbhm added a subscriber: lksbhm.Jul 12 2017, 4:49 AM

Very cool to see this working!

Correctly value-initialize the PluginInfo.

chandlerc added inline comments.Feb 28 2018, 3:43 AM
include/llvm/Support/Plugin.h
33 ↗(On Diff #108063)

This seems like a somewhat nasty layering...

If plugins fundamentally deal in pass builders, I wouldn't expect the plugin infrastructure to be in the Support library at all. I think the only reason the plugin stuff started off in the support library is because it relied exclusively on global ctors to connect to any functionality. I really like the idea of moving away from that for plugins and having a more proper callback mechanism, but I think it means we need to either use some abstraction or just make the plugin logic (somewhat) specific to *pass builder* plugins.

Beyond getting the layering and pass build stuff designed nicely, I suspect we should rope some folks more broadly in the community in to make sure the high level approach to plugins here makes sense long term... I'll try to ask around and see who has opinions here.

Improve Layering: Move New-PM Pass Plugins into the Passes library

[NFC] clang-format

chandlerc added inline comments.Mar 12 2018, 7:51 PM
include/llvm/Passes/PassPlugin.h
23 ↗(On Diff #136808)

We should have some documentation around when/where this should be incremented...

25–29 ↗(On Diff #136808)

We should probably stick this into Compiler.h

40 ↗(On Diff #136808)

Probably some documentation here would be good. ;]

Also, should this be weak so that it is OK if we never load a library that defines it?

44 ↗(On Diff #136808)

Out of curiosity -- why use a macro rather than just documenting that a plugin must define the above function?

include/llvm/Passes/PassPluginLoader.h
18 ↗(On Diff #136808)

This whole class needs doxygen comments. =D

Also, should it be called PassPlugin? And maybe be in the same header as above?

lib/Passes/PassPluginLoader.cpp
37–39 ↗(On Diff #136808)

I think this is indicative that optional isn't the right return type here... I think you want something more like llvm::Expected here so you can put the error message into return and make this more library friendly.

tools/opt/NewPMDriver.cpp
47 ↗(On Diff #136808)

maybe load-pass-plugin?

philip.pfaffe added inline comments.Mar 13 2018, 11:32 AM
include/llvm/Passes/PassPlugin.h
44 ↗(On Diff #136808)

Mostly so that clients don't forget the extern "C". I just thought it was convenient and liked the declarative style.

If you have strong feelings about this I can document it instead, I'm not really married to the idea.

include/llvm/Passes/PassPluginLoader.h
18 ↗(On Diff #136808)

I put the two classes in different headers because they have different clients. The PassPlugin.h header contains what's required on the client side, i.e. in the actual pass plugin. The Plugin class here is on the other hand only used by the driver to represent loaded plugins.

philip.pfaffe marked 5 inline comments as done.

Address review comments, mostly adding commentary and renaming things.

chandlerc added inline comments.Mar 26 2018, 7:32 PM
include/llvm/Passes/PassPlugin.h
25 ↗(On Diff #138387)

Here and below, we turn on Doxygen's auto-brief feature so I suspect most of the \brief tags can be dropped. This one might be the exception, I don't recall if autobrief works for \macro blocks or not.

55 ↗(On Diff #138387)

A separate question: should this be spelled to be more specific? I'm imagining: llvmGetPassPluginInfo. Similar question about the name of the struct as well -- should it be specific to this being a pass plugin?

44 ↗(On Diff #136808)

You can close the block-level extern "C" above after the type, and use a direct one here so that copy/paste of the declaration Just Works:

extern "C" PluginInfo LLVM_ATTRIBUTE_WEAK LLVM_PLUGIN_EXPORT llvmPluginGetInfo();

But maybe better to just give an example definition for copy/paste because I suspect it needn't be so long. At least, I would hope that the declaration here suffices to get the attributes and exports correct and the user can just:

extern "C" PluginInfo llvmPluginGetInfo() {
  ...
}

I'm always hesitant to generate code with a macro. It feels ... icky to me. Maybe that's just my bias.

include/llvm/Passes/PassPluginLoader.h
18 ↗(On Diff #136808)

I don't (personally) see a big problem with having these in one file. It seems unlikely to be a burden on the plugin author to have this code included and keeps it co-located.

I would try to use names that differentiate the loaded plugin object from the C-struct used by a plugin to describe itself. I'm not sure what the best naming pair is there....

philip.pfaffe marked 6 inline comments as done.

Rename, move around, and simplify some things.

Most importantly: Rename the plugin-provided callback descriptor to
PassPluginLibraryInfo, and the handle to the loaded library to
PassPlugin.

chandlerc requested changes to this revision.Apr 3 2018, 3:41 AM

This is looking really awesome. Starting to get into more mundane / nit picky comments below.

include/llvm/Passes/PassPlugin.h
66–67 ↗(On Diff #140067)

Rather than expose the info object, maybe expose nice APIs for everything that wrap it up?

93 ↗(On Diff #140067)

I don't know anything about Windows or DLLs really... But I would naively expect that *here* we want __declspec(dllimport), and we want plugins to use __declspec(dllexport). Is that not the case? I'm happy to just be told "nope, this is how it works" without even a comment. Mostly asking to learn.

lib/Passes/PassPlugin.cpp
20–21 ↗(On Diff #140067)

Bundle the error string provided by getPermanentLibrary here?

20–21 ↗(On Diff #140067)

This will be substantially more efficient if you cast the first component to a Twine as then the +s will be saved and evaluated at the end. Something like: Twine("foo") + a + "bar" + c is ideal when dealing with Twine operands (and that is how StringError is built).

38–40 ↗(On Diff #140067)

Here in particular using the Twine composition is likely especially good. You should be able to either directly pass the C-strings or build StringRef's around them and avoid any allocation at this layer.

tools/opt/NewPMDriver.cpp
31 ↗(On Diff #140067)

Stale header.

218 ↗(On Diff #140067)

Expand a bit? "Walk the pass plugins and let them register any pass builder callbacks." or some such.

221–223 ↗(On Diff #140067)

Generally LLVM style would suggest to continue here rather than using an else. Nto a big deal in this case.

unittests/Passes/Plugin.cxx
1 ↗(On Diff #140067)

.cpp?

Also, maybe "TestPlugin"?

unittests/Passes/PluginsTest.cpp
26–34 ↗(On Diff #140067)

Do we have any tests that do this successfully already?

I'm worried it will be a nightmare to get this to work portably. But it is a really awesome level of testing, so seems worth at least trying, just wanted you to be prepared for the potential churn required to get it to work.

37 ↗(On Diff #140067)

Use StringRef instead? Or see below...

This revision now requires changes to proceed.Apr 3 2018, 3:41 AM
philip.pfaffe added inline comments.Apr 3 2018, 3:50 AM
unittests/Passes/PluginsTest.cpp
26–34 ↗(On Diff #140067)

I think I copied this from unittests/Support/DynamicLibrary. Maybe it makes sense to factor this out into a common header in llvm/Testing?

philip.pfaffe marked 12 inline comments as done.

Address review comments:

  • Use more StringRef and Twine
  • Hide the PassPluginLibraryInfo object
chandlerc accepted this revision.Apr 5 2018, 1:50 AM

LGTM with doc fixes and Twine fixes below. Really excited about this!

Thanks for the ton of work getting us from zero plugins to rich plugins!

include/llvm/Passes/PassPlugin.h
37 ↗(On Diff #140946)

Maybe expand this a bit to explain this defines the core interface used to interact with plugins but is primarily for plugin implementors (as opposed to plugin users) and point users to the class below that loads and wraps the plugin?

53 ↗(On Diff #140946)

%Metadata?

lib/Passes/PassPlugin.cpp
38–40 ↗(On Diff #140067)

Rather than std::to_string, you can just call Twine on integers.

In fact, I think the operator+ overload will do the right thing here, and you can just add these together and it will do twine-based concatenation.

This revision is now accepted and ready to land.Apr 5 2018, 1:50 AM
This revision was automatically updated to reflect the committed changes.
miyuki added a subscriber: miyuki.Apr 17 2018, 9:08 AM

The PluginsTest test is failing in our builds:

/work/llvm/src/unittests/Passes/PluginsTest.cpp:42: Failure
Value of: !!Plugin
  Actual: false
Expected: true
Plugin path: /work/llvm/build/unittests/Passes/TestPlugin.so
Expected<T> must be checked before access or destruction.
Unchecked Expected<T> contained error:
Could not load library '/work/llvm/build/unittests/Passes/TestPlugin.so': /work/llvm/build/unittests/Passes/TestPlugin.so: undefined symbol: _ZN4llvm23EnableABIBreakingChecksE

This is probably related to the fact that we use -DLLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON. The host platform is x86_64-linux. Could you please check, if the test is supposed to work in such configuration.