This is an archive of the discontinued LLVM Phabricator instance.

[PM][WIP] Enable out-of-tree registration of passes with the new PassBuilder
ClosedPublic

Authored by philip.pfaffe on May 23 2017, 2:48 PM.

Details

Summary

This is approach tries different angle towards plugin pass registration.

Through the HookManager API, out-of-tree callers may register callbacks with the various stages at which passes are added into pass managers, including parsing of a pass pipeline as well as at extension points within the default -O pipelines.

Registering utilities like require<> and invalidate<> needs to be handled manually by the caller, but I've provided a helper for that.

I've marked this patch WIP because there is one key element missing currently, which is automatically detecting the type of a pipeline given just a pass name. I've yet to come up with a nice solution here. Obvious options are either adding another callback insertion point here, or registering the set of available names (with their IRUnit type); neither of which I don't like to much. I'm happy about suggestions.

Diff Detail

Event Timeline

philip.pfaffe created this revision.May 23 2017, 2:48 PM

I've merged the HookManager into the PassBuilder type. To rendezvous the PB with plugins, it is now able to call a well-known function extern "C" void RegisterPluginPasses(PassBuilder &) on the loaded objects. Plugins get to register callbacks there.

A linked-in Polly will not use this mechanism though, which is why I'm invoking it directly in opt's NewPMDriver as a demonstration (other tools will do the same).

chandlerc edited edge metadata.Jun 1 2017, 5:17 AM

First round of high-level feedback on this approach, thanks so much for working no this!

include/llvm/Passes/PassBuilder.h
541

Generally, I'd like to not add more transitions between public and private. I would prefer the following rough order:

0. Before the first public, any private types / aliases needed to define the public API.

  1. Public API
  2. Protected API (if it exists)
  3. Private API
542

Could you sort these based on the order in which they first are used in the default pipeline?

570–573

This seems redundant with OptimizerLast. I think in the new PM we should just have OptimizerLast and we should pass the OptimizationLevel as a parameter to *all* of these callbacks.

595–604

What do you think about naming these FooT-style? I don't feel strongly about it, but often like it to identify pure type aliases.

598–600

I think you should show an example of how to use this here.

598–602

I don't think it makes sense to have these be two different kinds of callbacks. At least, not until there are complaints about how annoying it is to ignore the rest.

I think your second signature is pretty good. It lets users that don't care about nested pipelines just use an unnamed parameter and ignore even the struct type, etc.

I would call this callback type something to do with *parsing* though as that is all it does.

603

Naming convention suggests fooBarBaz here.

Also, "handle" doesn't really help the reader much IMO. I would name it after the name of the callback above. IE, if above it is something like ParsePassCallbackT I would call this parseAnalysisUtilityPasses.

604

This should be called something to do with *registering*.

604

There is a type PipelineEntry. I don't think that makes a good parameter name here, especially when the parameter is extracted *from* that type...

625–632

I would suggest separating this into a later change. The extension mechanisms should work well out of the box for non-plugin use cases and it lets the review be a bit more focused.

As an example, I'd like to use some of this to implement support for using the new PM in Clang when running sanitizers.

640–643

Rather than all four of these plus the huge swath of extension point enums above, how do you feel about a different API:

All of the extension points above have a fixed pass manager type. So what if we have one registerFooCallback for each extension point, completely remove the enum from the public API, and make the pass manager type concrete and precise?

While this would mean more register... functions, not more than we already have enumerators. You should move the comments currently on the enumerators to be on the registration routines. I also generally think it would be best to document the extension points separately from the rest of the registration stuff as its much less boiler-plate-y.

One reason why I prefer this -- with the current API, there are a lot of combinations of pass manager types and enumerations that won't get any compile time error but cannot actually fire in practice.

654–680

Why are these public? I would make them implementation details. And in most cases they're called in only one place and could be completely inlined.

philip.pfaffe marked 12 inline comments as done.

Addressed review comments.

The two main things are:

  • Remove the extension point enum and replace it with individual callbacks
  • Get rid of the invoke() functions and inline them everywhere
davide added a subscriber: davide.Jun 1 2017, 2:33 PM
mehdi_amini added inline comments.Jun 1 2017, 5:06 PM
include/llvm/Passes/PassBuilder.h
178

That's a long block of undocumented code here.

Generally this is starting to get pretty close. What are your thoughts on testing? I would suggest a unittest because that should be a good way to simulate an "out of tree" user.

include/llvm/Passes/PassBuilder.h
49

Now that this is public, probably need to flesh out its documentation so folks understand the details of how this should be expected to work.

148–149

I think this template and all the using aliases of it below don't really help readability enough. These types only show up in one place (the declaration of a function that has a nice comment) and should just be written out in that location IMO.

178

Actually, I think all of these are used in essentially two places: a register signature and the vector declaration below. I'm not sure adding an alias just to save that single repeatition is worth it.

I actually find the public functions more readable without the type alias, even though it means a mild amount of repeatition in the container type at the bottom. What do you think Mehdi?

456

This one deserves its own dedicated comment rather than being grouped.

457–463

I would separate the analysis callbacks from the pipeline parsing callbacks and give them (each) their own documentation.

468–469

This one too deserves its own documentation IMO.

lib/Passes/PassBuilder.cpp
284–287

Since these are all one-line functions, would it make sense to inline them into the header so we don't have to re-declare all of them?

911–915

SimplifyFPM -> PeepholeFPM ?

934–937

We should factor out a helper that adds the peephole passes to an FPM as we do this in a lot of places.

philip.pfaffe marked 10 inline comments as done.

Address Review comments

This contains one functional change: I've moved the parseXPassPipeline() functions into the public API, and added the PB instance to the TopLevel parsing callback. The reason is that when I'm being invoked for the TopLevel pipeline, I usually don't actually want to handle all the parsing myself. Instead, I'd only like to set up the PM stack (either by messing with the PipelineElement nest, as PB does, or prepare the stack of PM objects), and then hand this back into the PassBuilder.

Also:

  • Inline CallbackT using-decls as well as the function definitions into the callback registration functions in the header.
  • Commentary
grosser added a subscriber: grosser.Jun 2 2017, 2:54 AM

Regarding Testing:
The unit-test included is a bit stale at the moment, because I didn't keep up with the API changes and extensions. Once everyone is happy with the general callback registration API, I'll flesh it out to also test different IRUnits and the things like extension points and top level pipelines. Moreover, currently it only checks if PassBuilder is happy with the pipeline, it doesn't actually verify that the PassManagers are correctly populated.

As per Chandler's out-of-band suggestion, remove the parseXPassPipeline() functions from the public API again. Instead, use the existing parseXPipelineCallbacks to determine if a callback accepts a pass name for IRUnit X.

Your testing strategy looks good. Comments below. I think this is getting close enough that it makes sense to start hacking on tests for it. Thanks so much for all the work here, I really like how this is turning out!

include/llvm/Passes/PassBuilder.h
385

Here and below, can drop the old enumerator before the '-'.

387–401

I think we should drop both of these "early" extension points and the "last" extension point.

In the new PM, it is trivial to nest a pass manager, allowing users to easily run passes both before and after the pipeline. This should reduce the number of extension points we have to carry and it removes a semantic complication with the "last" extension point. If we really can't get any of the users of these ported over, we can add them back, but I'd like to be somewhat lazy here and work to address these use cases with nesting and wrapping as long as possible.

482

I would expand on this a bit.... You should spell out what 'AA' stands for here, and talk about what the callback is expected to do with the AAManager.

501

I think registerAnalysisRegistrationCallback reads slightly better... But not a lot...

520

Similarly registerPipelineParsingCallback. Curious if others agree though.

lib/Passes/PassBuilder.cpp
1070–1090

Here and below, I would just s/CallbacksArrayT/CallbacksT/g. I don't think Array is really helping... You really mean more Range, but I think plural is enough to signify "hey I can loop over it".

1070–1091

Probably worth a comment explaining what craziness we're doing here and why. =]

1285–1293

This restructuring seems bad to me...

I think we always want to run the callbacks *last*. The way the parsing logic works, there are two "last" places (one when we have an inner pipeline, one without).

If you're bothered by duplicating the for loop, you could put this in a tiny lambda you declare at the top, and just call it on each path?

1668–1672

If we're going to brace one arm, let's brace them all here.

philip.pfaffe marked 8 inline comments as done.

Address (most) review comments.

  • Mostly comments and rename a few things.
  • Move parsing callback invocations to after the builtin passes

Running callbacks last during parsing last means that callers are now unable to "steal" pass names.

philip.pfaffe added inline comments.Jun 6 2017, 3:28 PM
include/llvm/Passes/PassBuilder.h
387–401

I'm not entirely convinced about this. I expect most users of the callbacks won't have access to the outermost PassManagers, so nesting will not be easily possible.

lksbhm added a subscriber: lksbhm.Jun 7 2017, 7:47 AM

Flesh out the tests. Mainly:

  • Expand the unittest to handle all IRUnits as well as parsing of a top-level pipeline
  • Add a lit test for the extension points.

To enable the latter, I added a few command line options to opt which accept textual pipelines (for appropriate pass managers), which then get inserted into extensions points. This requires 3 additional APIs on the PassBuilder for parsing a pipeline text into something other than a ModulePassManager.

Whew, back to this.

Mostly nit picks and comments about testing.

include/llvm/Passes/PassBuilder.h
387–401

How so? I'm curious what the issue is here... Callbacks seem like the most complex form of extension, whereas nesting seems much simpler...

486–495

I would sink this to after the other pipeline parsing callback registration entry points.

548

EP -> Extension Point.

lib/Passes/PassBuilder.cpp
988

Here especially (but also elsewhere) I'd put the invokePeepholeEP... line adjacent to the InstCombinePass line (they're the two peepholes) rather than next to the subsequent stuff which is unrelated.

1315

No need for blank line here.

1563–1566

Should this go after we parse the builtin in AA names?

1669

No need for blank line.

1689–1698

I'm not sure you want this fallback logic here...

If you want to be able to essentially duplicate the above logic, you want this to *only* succeed at parsing a CGSCC pass pipeline.

Otherwise, you'll never be able to skip the CGSCC layer when you get a function pass (or something nested inside a function pass) and you're trying to build the module layer thing.

1712–1719

While less important, same comment as above.

test/Other/new-pm-defaults.ll
29–33

What about having just one opt invocation with a no-op pass in *every* extension point? I don't think you lose any coverage, and then you have a much easier check prefix of just CHECK-EP.

tools/opt/NewPMDriver.cpp
101–105

Do the polly bits in a follow-up commit?

unittests/IR/HookManagerTest.cpp
23 ↗(On Diff #102210)

Rather than counting things, I would suggest using gmock for this. It is kind of what gmock was built for. To see how to get value-type wrappers around mocks that you can set expectations on, look at unittests/Transforms/Scalar/LoopPassManagerTest.cpp. You should end up with very similar code to this to template out the mocks over the types you care about, but should be able to use things like EXPECT_CALL to track when 'invalidate' or 'run' are called which seems way better than counting.

83–113 ↗(On Diff #102210)

I think you can inline all this as lambas below? You shouldn't need a generic lambda because you should have the types inside that template already.

122–127 ↗(On Diff #102210)

Throughout this test, I would try to comment a bit more heavily. My suggestion would be the following: assume the reader is trying to learn how to use the extension point interface to write their own customizations, and they are relatively unfamiliar with the extension point API, googletest (especially typed tests) and goolemock. You want to have enough comments that they can read this code cold and use it as documentation for how the API should work in practice (as opposed to its spec).

Also, probably need a better name than HookManagerTest?

philip.pfaffe marked 11 inline comments as done.

Addressed (most of the) review comments:

  • Drop Early and Late EPs
  • Fix nits
  • Convert the unittest to gmock

Two things are still open and require more discussion:

  1. The overloads of parsePassPipeline. These functions implement top level parsing. I think we want automatic nesting inference in all these cases. A use case is for example this: opt can now parse EP pipelines into the various EPs, most of which do not construct a ModulePassManager. Consider running opt -passes-ep-peephole="licm". The Peephole EP inserts passes into a FunctionPassManager, and as with the -passes option, this simplifies typing out the pipeline a lot.
  2. A single opt invocation in the new-pm-defaults.ll testcase: Making this a single opt RUN line would certainly simplify the testcase a bit. What I like about the way this looks now, however, is that the CHECK-EP-* prefixes show very clearly what and where the extensions points are.

Pretty desperately need to factor the mocking helpers out, but that's not your fault. The actual test code is really clean IMO.

As discussed in IRC, I'd still like to remove the synthesis of inner IR-unit pipelines for EPs and remove the extra parsing APIs. I don't think they pull their weight. But the opt check is fine with a tweak below.

More detailed comments on the unittest mostly.

include/llvm/Passes/PassBuilder.h
478

calles -> callers

test/Other/new-pm-defaults.ll
29–33

I see, you want to check the *different* extension points.

OK, in this case, I would still name the check prefixes using ALL-CAPS-WITH-DASHES rather than the mixed case you have.

unittests/IR/PassBuilderCallbacksTest.cpp
160

API grouping doxygen doesn't seem terribly useful inside the bowels of the test.

271

You can't name identifiers with leading _ and a capital letter.

Perhaps TestIRUnitT?

296–301

I would inline this into the caller so you can write more precise expectations.

For example:

ASSERT_TRUE(PB.parsePassPipeline(this->PM, PipelineText, true)) << PipelineText;

Seems likely to give a much nicer error when it fails. =]

I guess you could provide wrappers around PB.parsePassPipeline to ease some of the code, but it's not clear it is worth it. The test should be easy to inspect even if a very minor of repetition is involved.

Also, do you need the this-> here?

312–313

Why local_unnamed_addr? Seems like this test should really be as simple as possible?

philip.pfaffe marked 6 inline comments as done.

Addressed Comments

chandlerc accepted this revision.Jul 9 2017, 12:21 PM

LGTM

I have some thoughts about simplifying the test, but let's ship this. I think those can be done incrementally afterward. Thanks again for all the work here!

test/Other/new-pm-lto-defaults.ll
20

nit: Can omit blank line I think?

This revision is now accepted and ready to land.Jul 9 2017, 12:21 PM

Very nice! Thanks to everyone for working on this!

This revision was automatically updated to reflect the committed changes.