This is an archive of the discontinued LLVM Phabricator instance.

[New PM] Introducing PassInstrumentation framework
ClosedPublic

Authored by fedor.sergeev on Jun 6 2018, 4:56 PM.

Details

Summary

Pass Execution Instrumentation interface enables customizable instrumentation
of pass execution, as per "RFC: Pass Execution Instrumentation interface"
posted 06/07/2018 on llvm-dev@

The intent is to provide a common machinery to implement all
the pass-execution-debugging features like print-before/after,
opt-bisect, time-passes etc.

Here we get a basic implementation consisting of:

  • PassInstrumentationCallbacks class that handles registration of callbacks and access to them.
  • PassInstrumentation class that handles instrumentation-point interfaces that call into PassInstrumentationCallbacks.
  • Callbacks accept StringRef which is just a name of the Pass right now. There were some ideas to pass an opaque wrapper for the pointer to pass instance, however it appears that pointer does not actually identify the instance (adaptors and managers might have the same address with the pass they govern). Hence it was decided to go simple for now and then later decide on what the proper mental model of identifying a "pass in a phase of pipeline" is.
  • Callbacks accept llvm::Any serving as a wrapper for const IRUnit*, to remove direct dependencies on different IRUnits (e.g. Analyses).
  • PassInstrumentationAnalysis analysis is explicitly requested from PassManager through usual AnalysisManager::getResult. All pass managers were updated to run that to get PassInstrumentation object for instrumentation calls.
  • Using tuples/index_sequence getAnalysisResult helper to extract generic AnalysisManager's extra args out of a generic PassManager's extra args. This is the only way I was able to explicitly run getResult for PassInstrumentationAnalysis out of a generic code like PassManager::run or RepeatedPass::run.
  • PassBuilder takes PassInstrumentationCallbacks object to pass it further into PassInstrumentationAnalysis. Callbacks registration should be performed directly through PassInstrumentationCallbacks.
  • new-pm tests updated to account for PassInstrumentationAnalysis being run
  • Added PassInstrumentation tests to PassBuilderCallbacks unit tests. Other unit tests updated with registration of the now-required PassInstrumentationAnalysis.

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
fedor.sergeev planned changes to this revision.Aug 27 2018, 2:49 AM

Will add more comments! :)

include/llvm/IR/PassInstrumentation.h
43 ↗(On Diff #162636)

Nope, this is left over from some previous version.

adding comments

Back from vacation, and had time for a closer look.

My major concern right now is ownership. In your current state, PassBuilder owns both all the callbacks as well as the state of the instrumentation (i.e., the counter). As it were, PassBuilder is in fact more of a transient factory that you can throw a way after you've set up your pipeline. Or you might even use it to make more than one pipeline. In both cases ownership is a problem.

Instead, I believe it makes for nicer separation if PassBuilder becomes purely the entry point for callback registration, and all bookkeeping happens within the pipeline. IMO, a good place to put this would be the Module-level Analysis itself, which is a singleton wrt. a single compilation. Federation between different IRUnits can happen via proxies, which adds some amount of indirections, but I still think that's manageable.

Lastly, I think this deserves unittests. You can check out PassBuilderCallbacksTests or LoopPassManagerTests for inspiration on how to mock out passes and analyses. This might also be a perfect moment to nudge @chandlerc in the direction of D39678 to simplify this :)

fedor.sergeev marked 4 inline comments as done.Aug 29 2018, 9:58 AM

Back from vacation, and had time for a closer look.

My major concern right now is ownership.

Yes, this is one of the main concerns for me as well.

In your current state, PassBuilder owns both all the callbacks as well as the state of the instrumentation (i.e., the counter). As it were, PassBuilder is in fact more of a transient factory that you can throw a way after you've set up your pipeline. Or you might even use it to make more than one pipeline. In both cases ownership is a problem.

This is exactly when submitting my RFC I was going to use LLVMContext for this ownership.
Some of replies (including yours ;) - http://lists.llvm.org/pipermail/llvm-dev/2018-June/123850.html) were in favor of PassBuilder.
PassBuilder is not ideal for my own downstream project either - we do not keep PassBuilder around.
But then we need to find something else that owns.

Instead, I believe it makes for nicer separation if PassBuilder becomes purely the entry point for callback registration, and all bookkeeping happens within the pipeline. IMO, a good place to put this would be the Module-level Analysis itself, which is a singleton wrt. a single compilation.

Ehm... let me try this. When I quick-checked it last time I did not like amount of changes I need to do for that. But it is doable and I'm definitely more familiar with codebase as of right now.

Federation between different IRUnits can happen via proxies, which adds some amount of indirections, but I still think that's manageable.

I'm not particularly happy with all these PassInstrumentationAnalysis runs, for one because it makes updating all the new-pass-manager tests that use -debug-pass-manger quite a churn. :)

Lastly, I think this deserves unittests. You can check out PassBuilderCallbacksTests or LoopPassManagerTests for inspiration on how to mock out passes and analyses.

Definitely. I just had no experience with llvm unit-tests before that, so I decided to postpone doing them until after we settle on main design points.

One additional implication on design that I just realized:

-pass-times functionality effectively requires to differentiate between distinct Pass instances, separately tracking times for different instances of the same "pass type".
That in turn requires passing some unique identifier for the pass instance, not just for the pass type as I was doing before.

And that means there should be extra parameter to all the pass callbacks taking either pointer to pass (void*???) or some encoded version of that (unique string etc).
Perhaps it should be part of PassExecutionCounter (which actually becomes PassExecutionID and include both "execution counter" and "pass instance ID").

philip.pfaffe added a comment.EditedSep 3 2018, 3:41 AM

IMO passing void* as an opaque ID along with the name is fine. Passing the PassConcept by reference would degrade layering, since it lives in IR, not in Passes like the rest of this. could be a viable alternative, the Passes library depends on IR anyways.

Edit: Although it's doable, I again don't like handing out access to the pass in order to prevent clients from running the pass themselves.

Reconsidering passing the actual IRUnit to the callbacks, I still think that's not a good idea. Downstream users _will_ eventually use this to hook into the pipeline and mess with the IR somehow. So instead this should be opaque to clients.

What operations do clients ever have to perform on an IRUnit? The number should be tiny I believe.

I'd love to see something like a generic IRUnit wrapper, that implements these operations plus some opaque IRUnit id. Currently that'd just be print. Passing the wrapper to the callback then is safe, and it allows clients to both identify the ir object as well as do what we allow them to do with the object.

Reconsidering passing the actual IRUnit to the callbacks, I still think that's not a good idea. Downstream users _will_ eventually use this to hook into the pipeline and mess with the IR somehow. So instead this should be opaque to clients.

I dont fully get your concerns.
By analogy - downstream users can use their own Analyses to mess with IR. But we do not have any kind of protection against that.
Does it worry you the same way?

What operations do clients ever have to perform on an IRUnit? The number should be tiny I believe.
I'd love to see something like a generic IRUnit wrapper, that implements these operations plus some opaque IRUnit id. Currently that'd just be print.

Printing is the first user, yes.
Any kind of inspection for debug purposes (like putting IR into database).

Passing the wrapper to the callback then is safe, and it allows clients to both identify the ir object as well as do what we allow them to do with the object.

One of the problems with generic IRUnit wrapper is that it is not easily extensible.
A prime idea of pass instrumentation is to provide an easily extensible pass-pipeline observability tool.
I would not like to harm extensibility of this tool for the sake of protecting users against foot-shooting themselves.

What operations do clients ever have to perform on an IRUnit? The number should be tiny I believe.

Looking at the original RFC I see:

-print-after
-verify-each

plus ideas like:

-git-commit-after-all
-debugify-each

By a pure accident I left out the IRUnit parameter in the first version of RFC but did correct myself soon as after.
I was under impression that suggested non-opaque nature of IRUnit is kinda clear during the discussion that followed
(though nobody was specifically picking on it pro- or contra-).

IMO passing void* as an opaque ID along with the name is fine. Passing the PassConcept by reference would degrade layering, since it lives in IR, not in Passes like the rest of this. could be a viable alternative, the Passes library depends on IR anyways.

Yes, idea is to use void* of the PassConcept as an opaque ID.
Actually I plan to hide it under struct PassInstanceID, and its implementation can choose other ways of hiding this (e.g using unique string or whatnot).

Okay, that sounds reasonable.

Okay, that sounds reasonable.

Eh... perhps I was too chatty here...
Can you confirm what among my mumblings appeared reasonable to you, all of it? :)

Sorry :)

I meant both your arguments towards passing the real IRUnit and an opaque pass identifier.

philip.pfaffe added inline comments.Sep 3 2018, 12:45 PM
include/llvm/IR/PassManager.h
709 ↗(On Diff #162648)

Are you ever actually calling this, except through the indirection above?

fedor.sergeev added inline comments.Sep 3 2018, 1:36 PM
include/llvm/IR/PassManager.h
709 ↗(On Diff #162648)

Any suggestions on how to simplify it?
This unpacker thing is the only way I figured to get Ns == sizeof(ExtraArgTs) and not sizeof(ArgTs).

philip.pfaffe added inline comments.Sep 3 2018, 1:41 PM
include/llvm/IR/PassManager.h
709 ↗(On Diff #162648)

You can do Ns == sizeof...(ExtraArgTs), but you're not actually using that here?

fedor.sergeev added inline comments.Sep 3 2018, 2:07 PM
include/llvm/IR/PassManager.h
709 ↗(On Diff #162648)

This is exactly a purpose of these two getPassInstrumentation methods that take tuple.
See in PassManager::run() :

PassInstrumentation<IRUnitT> PI =
    AM.getPassInstrumentation(IR, std::tuple<ExtraArgTs...>(ExtraArgs...));

Here ExtraArgTs are PassManager's ExtraArgTs and they do not match AnalysisManager's ExtraArgTs.
I need to take PassManager's ExtraArgs, deduce AnalysisManager's ExtraArgTs (different ones) from AM parameter.
This deduction is what first helper is for.
And then it calls a second helper that takes first N arguments from tuple.

philip.pfaffe added inline comments.Sep 3 2018, 2:25 PM
include/llvm/IR/PassManager.h
709 ↗(On Diff #162648)

Right! Looks like i missed the difference between ArgTs and ExtraArgTs.

In this update:

  • PassInstanceID introduced to track pass instances
  • IRUnits are const references to avoid mishaps in instrumentation callbacks
  • some comments added/updated
fedor.sergeev edited the summary of this revision. (Show Details)Sep 4 2018, 7:04 AM

switching to simpler PassInstanceID constructors - overload instead of sfinae

deleting a minor stray change that belongs elsewhere

Harbormaster completed remote builds in B22226: Diff 163901.
philip.pfaffe added a comment.EditedSep 5 2018, 3:07 AM

Here's what I think the ownership layering should look like:

struct PassInstrumentationProxy : AnalysisInfoMixin<> {                         
  using Result = PassInstrumentation *;                                         
                                                                                
  Restul run(...) { return AM->PI; }                                            
};

struct PassBuilder {                                                            
  PassInstrumentation *PI = nullptr;               
  PassBuilder(PassInstrumentation *PI = nullptr) : PI(PI) {}
                                                                                
  void addCallback(...);                                                        
                                                                                
  void registerFunctionAnalyses(ModuleAnalysisManager &MAM) {                   
    MAM.addPass(PassInstrumentationProxy(PI));
    ...                                                                         
  }                           
};                                                                              
                                                                                
struct PassInstrumentation {                                                    
  // Callbacks                                                                  
  SmallVector<std::function<BeforePassFunc>, 4> BeforePassCallbacks;            
  SmallVector<std::function<AfterPassFunc>, 4> AfterPassCallbacks;              
  SmallVector<std::function<void()>, 2> StartPipelineCallbacks;                 
  SmallVector<std::function<void()>, 2> EndPipelineCallbacks;                   
                                                                                
  // Instrumentation payload                                                    
  unsigned PassExecutionCounter;                                                
};

PassBuilder becomes purely the callback registration entrypoint. When populating any AM, all the callbacks get passed down into it. Passes and passmanagers access the payload through a proxy analysis.

Edit: Incorporated some suggestions by Chandler.

Ok, to summarize what I'm going to change:

  • drop the pimpl proxy in PassInstrumentation/PassInstrumentationImpl
  • make it a single class, no templates
  • ownership outside of PassBuilder/PassManager
  • pass to PassBuilder, non-owning reference, registration through PassBuilder
  • modify RepeatedPass::run to make it easier to use getResult
  • getResult returns pointer to PassInstrumentation

PassInstrumentation handling simplified - now it is just a single class,
Ownership is not managed by PassBuilder anymore.
A bit of renaming (e.g. PassInstrumentationAnalysis -> PassInstrumentationProxy).

fedor.sergeev edited the summary of this revision. (Show Details)Sep 6 2018, 9:08 AM
fedor.sergeev edited the summary of this revision. (Show Details)

I like how simple this is now!

include/llvm/IR/PassInstrumentation.h
199 ↗(On Diff #164223)

Is mutable necessary now?

include/llvm/IR/PassManager.h
919 ↗(On Diff #164223)

Empty line?

1209 ↗(On Diff #164223)

No need to make this a template class! Since PassInstrumentation isn't, anymore, this doesn't need to be either.

include/llvm/Passes/PassBuilder.h
568 ↗(On Diff #164223)

I don't think this should assert. This can be called from a plugin context, and the plugin has no chance of knowing whether PassInstrumentation is available. So in that case, just do nothing I guess.

fedor.sergeev added inline comments.Sep 6 2018, 11:37 AM
include/llvm/IR/PassInstrumentation.h
199 ↗(On Diff #164223)

Yes, it is necessary since runBeforePass - the only callback that increments PC - is const.
I'm not sure it buys us anything, so I can get rid of both const there and mutable here.
I dont care actually :)

addressing comments

fedor.sergeev marked 5 inline comments as done.

typo

moving PassInstrumentationProxy definition around (needed for subsequent changes
in AnalysisManager).

adding unit tests

This is looking really, really good. Some comments below.

include/llvm/IR/PassInstrumentation.h
97–107 ↗(On Diff #165165)

There is a lot of complexity here which seems largely designed to try and produce the opaque pointer. Especially as that pointer isn't really "reliable", is this really that important?

What breaks if we simply give the callback the name of the pass? Then we can just have the instrumentation object have templates that take a generic PassT &P and then do P.name() to get the name. This should work both with concrete passes and with all of the PassConcept objects (the real case).

I think the tuple of (name, IR unit) is about the most precise we can make the identity in the API.

This doesn't prevent the instrumentation callback from doing things like keeping counters and such internally to differentiate what is going on....

195 ↗(On Diff #165165)

Why expose this? We can trivially implement the empty case anyways?

198 ↗(On Diff #165165)

Why include this here? Seems like the counter stuff could be separate and just managed by the callbacks when wired up?

include/llvm/IR/PassManager.h
465 ↗(On Diff #165165)

Now that we have multiple uses of Passes[Idx], I would hoist it into a variable.

543 ↗(On Diff #165165)

No need for _ here (or anywhere else here). (And _PI is a reserved name.)

760–768 ↗(On Diff #165165)

This coupling with the analysis manager seems really awkward. It makes it no longer "just an analysis". I'd really like to get back to that.

I see at least two ways to do this:

  1. just bite the bullet and require that if you use an analysis manager, you have to register the instrumentation
  2. Write a utility function that implements this but isn't part of the analysis manager.

I lean slightly toward #1, but maybe its way more awkward than I'm imagining? It would have the advantage of simplifying the code because yo uwould "always" have a (potentially trivial) instrumentation object.

fedor.sergeev added inline comments.Sep 13 2018, 4:25 AM
include/llvm/IR/PassInstrumentation.h
97–107 ↗(On Diff #165165)

The problem is that "name" is not going to give callbacks a way to differentiate between *instances* of passes.
And, say, -time-passes functionality needs to collect timings per-instance.

195 ↗(On Diff #165165)

The intent is to avoid overhead of instrumentation framework as much as possible for the case when there are no callbacks registered.
Current implementation provides nullptr to the proxy analysis instead of the empty Instrumentation object.
It saves some minor cycles on dereferencing and iteration through empty vectors.

First implementation had no need for empty() since PassBuilder was managing the object and avoided creating it altogether if no registrations requests were given. Current implementation manages PassInstrumentation outside of PassBuilder, so PassBuilder needs empty().

198 ↗(On Diff #165165)

Oh, well... had an idea on making counters independent from callbacks decisions on skipping some passes, but then realized it is hardly possible overall. So yes, it can be made separate.
And then I hardly need to introduce this class at all here.

include/llvm/IR/PassManager.h
760–768 ↗(On Diff #165165)

"just an analysis" is perhaps not fully true anyway, since it is the only analysis that is being explicitly queried in all the generic parts of the code, even in generic PassManager.
If we decide #1 then we essentially legalize it not being "just an analysis". And I'm fine with it.
It will require adding registration to all the PassManager tests that do not use PassBuilder currently (not a big deal either, just a hassle).

philip.pfaffe added inline comments.Sep 13 2018, 4:33 AM
include/llvm/IR/PassInstrumentation.h
97–107 ↗(On Diff #165165)

How would a callback with a template argument look like, and how would you store it?

Can you do it without another Concept/Model pair capturing the Pass interface? Because I don't think that'd save a lot in terms of complexity.

include/llvm/IR/PassManager.h
760–768 ↗(On Diff #165165)

I'd +1 the first way, too. When using a PassManager most of the time you're not free to run around with an empty AnalysisManager anyways, so the burden would be low, IMO.

chandlerc added inline comments.Sep 13 2018, 5:11 AM
include/llvm/IR/PassInstrumentation.h
97–107 ↗(On Diff #165165)

It's not clear that time-passes needs this functionality.

My problem is that I think this overpromises something that we cannot actually deliver: This cannot capture the *iteration* count either, and that is just as much of a factor as the position in the pipeline.

If we want detailed statistics for timing, we should collect the time of *each* run (and the IR unit it ran over) and show them separately. For summary statistics, I think it just being the name is good in that it sets reasonable expectations.

97–107 ↗(On Diff #165165)

The method on PassInstrumentation can be the template, and the callback just gets the StringRef for the name.

195 ↗(On Diff #165165)

I thikn you should keep simplifying.

The cost of an empty for loop in the instrumentation is going to be tiny. We shouldn't add complexity to "avoid" it, especially when that complexity is a wider interface.

198 ↗(On Diff #165165)

It still seems useful to hold the callbacks and handle the dispatch (with a template).

include/llvm/IR/PassManager.h
760–768 ↗(On Diff #165165)

Yeah, the more I think, the more this can just be an analysis.

We can have a default constructor that leaves all the callbacks empty. It won't do anything interesting. But the code will be simple and work.

Then users can register more interesting ones as they like.

updating a solution as per review comments as well as following
the design discussion we had on IRC.
Main directions:

  • PassInstrumentation is split in two parts - callbacks/instrumenting points
  • PassInstrumentationAnalysis returns PassInstrumentation
  • no more PassCounter tracking in PassInstrumentation
  • PassInstanceID is gone, not trying to track pass instance as we have no good mental model on what it actually is. Passing just StringRefs - pass names.
  • using getResult<PassInstrumentationAnalysis> instead of getPassInstrumentation whenever possible (still using std::tuple variant in places where it needs to chop off AM args).

Most likely I forgot to mention something :)

fedor.sergeev edited the summary of this revision. (Show Details)Sep 13 2018, 11:10 PM
fedor.sergeev added a reviewer: philip.pfaffe.
fedor.sergeev edited the summary of this revision. (Show Details)

clang-formatting

This is looking great. Apart from two things that aren't needed anymore, the last point of discussion is getPassInstrumentation. PassInstrumentationAnalysis shouldn't be a special citizen. What about making this a free function instead? In fact, why not make this a generic helper e.g. in the detail namespace to query _any_ analysis result from a template pass?

include/llvm/IR/PassInstrumentation.h
70 ↗(On Diff #165478)

Is this still required?

146 ↗(On Diff #165478)

Is this necessary?

include/llvm/IR/PassManagerInternal.h
89 ↗(On Diff #165478)

Is this still required?

adding const to PassInstrumentation::runAfterPass

fedor.sergeev added inline comments.Sep 14 2018, 6:34 AM
include/llvm/IR/PassInstrumentation.h
70 ↗(On Diff #165478)

if I got Chandler right he asked me to keep it around for individual callbacks use.
I'm really not sure that it is needed anymore.

146 ↗(On Diff #165478)

you mean the default copy? perhaps not. will delete it.

fedor.sergeev added inline comments.Sep 14 2018, 6:34 AM
include/llvm/IR/PassManagerInternal.h
89 ↗(On Diff #165478)

definitely not

addressing comments, moving getPassInstrumentation tuple helper into detail::getAnalysisResult
free function.

At this point I'm bikeshedding: Should getAnalysisResultgo into the Internal header?

At this point I'm bikeshedding: Should getAnalysisResultgo into the Internal header?

Hmm.. I have no preference here. I can easily do that.
Chandler?

fedor.sergeev edited the summary of this revision. (Show Details)Sep 14 2018, 11:06 AM
fedor.sergeev edited the summary of this revision. (Show Details)

It gets better and better. Another round of comments.

include/llvm/IR/PassInstrumentation.h
70 ↗(On Diff #165478)

Sorry for ambiguity. I think you can pull it out for now. I would re-introduce it as part of the bisection in a way that fits with that framework.

16 ↗(On Diff #165518)

"IT is light on copy since it is queries" -> "It is cheap to copy since it queries"

17 ↗(On Diff #165518)

Not sure this explanation of why the copy is cheap quite parses for me.

20 ↗(On Diff #165518)

extraneous -.

107–113 ↗(On Diff #165518)

I think this is one of those weird places and times where emplace and such is what you want.

Specifically, I think you should do:

template <typename CallableT> void registerAfterPassCallback(CallableT C) {
  AfterPassCallback.emplace_back(std::move(C));
}

Or something of the sort. You want to support passing a lambda w/ move only captures and not allocating memory for it in the caller, but only inside the vector.

120–131 ↗(On Diff #165518)

Since you already befriend this class, why not just inline thes functions into their callers below?

133 ↗(On Diff #165518)

I don't think this comment is really necessary.

134–135 ↗(On Diff #165518)

I would suggest using llvm::unique_function throughout this code to support callbacks with move-only captures.

I generally suspect we should default to llvm::unique_function over std::function at this point until a need for copy arises.

144–145 ↗(On Diff #165518)

Comment on ownership and lifetime?

include/llvm/IR/PassManager.h
484–490 ↗(On Diff #165518)

Rather than assume we can do this generically with tuple-hacking of the parameter pack, I'd suggest simply assuming that the extra arguments match, and insisting for cases where they do not match we use explicit specializations of the relevant run method there to manually implement the logic.

We already do some of this. If it truly becomes problematic, I think we could solve it a different way instead, but I'm not sure it is worthwhile without seeing how many (and how much logic) ends up being duplicated w/ the specializations.

The reason I prefer specializations is that it places no firm contract on the relationship between the arguments. I'd prefer to not bake in such a relationship to the API because then we may end up designing things in awkward ways to satisfy this constraint.

568–571 ↗(On Diff #165518)

I think we should either move all the instrumentation stuff into this header, or find a way to pull this into the separate header. I think the latter is possible but will require some refactorings. Can you leave a FIXME for now so that we come back to this?

578 ↗(On Diff #165518)

I think a name like Callbacks would be easier to read than PIC.

762–764 ↗(On Diff #165518)

Is this still needed?

include/llvm/IR/PassManagerInternal.h
51 ↗(On Diff #165518)

This and all of the other changes to PassManagerInternal.h are really orthogonal cleanups and LGTM. Please pull them into their own patch and go ahead and land that.

include/llvm/Passes/PassBuilder.h
63–65 ↗(On Diff #165518)

Since the code constructing this has access to the callbacks already, why do we need to expose it? I'd rather keep the API narrow (like we do for TM above).

564–568 ↗(On Diff #165518)

I'm somewhat curious what the expected usage of this method is? It isn't obvious to me at all...

lib/Passes/PassBuilder.cpp
318–320 ↗(On Diff #165518)

No need for any of this -- you can use the PassRegistrsy.def and write PassInstrumentationAnalysis(PIC). That's how we already do things with TM.

unittests/IR/PassBuilderCallbacksTest.cpp
268–272 ↗(On Diff #165518)

Instead of this, you can use ::testing::NiceMock<MockPassInstrumentationCallbacks> in places where you want to ignore these. The way you've written it, even if users *want* to check for unexpected calls to these routines, they won't be able to do so.

283–284 ↗(On Diff #165518)

Maybe leave a FIXME here?

We should design a good matcher for llvm::Any and then we should be able to compose things. But this is fine for now.

301 ↗(On Diff #165518)

Missing vertical space.

423–424 ↗(On Diff #165518)

std::bind should generally be avoided at this point. We can just use lambdas to delegate here.

And see my comment on registerPassInstrumentian above: I think instead we should just register the callbacks first, and hand the object into the pass builder.

Notably, I think you can have the mock *contain* a PassInstrumentationCallbacks object that it pre-registers with lambdas that delegate to the mock, and then just pass that into PassBuilder constructor.

463–464 ↗(On Diff #165518)

I find it simpler to define a Sequence object and then connect it to each expectation with the .InSequence methods. This makes it fairly easy to avoid a proliferation of scopes to wire these things up.

You'll also want to say how *many* calls here, which will make more sense once you make the change I suggest above w.r.t. the number.

Thanks for a deep review.

include/llvm/IR/PassInstrumentation.h
17 ↗(On Diff #165518)

It doesnt quite parse for me as well :)
Pehaps I meant "since it is being queried through the analysis framework".
Will try to reword it in a more straightforward way.

134–135 ↗(On Diff #165518)

hmm... did not know about llvm::unique_function.

include/llvm/IR/PassManager.h
568–571 ↗(On Diff #165518)

I'm afraid I dont quite follow. Pull *what* into the separate header?
PassInstrumentationAnalysis definition altogether? or anything else?

And I dont like the idea of putting instrumentation stuff into this header.
It is already quite packed with different concepts.
Instrumentation looks distinct enough to deserve its own header.

include/llvm/Passes/PassBuilder.h
564–568 ↗(On Diff #165518)

It allows to use PassBuilder as sort of a central place for registering all the callbacks, w/o polluting Passbuilder with all the interfaces.
Particularly useful for plugins, I believe.
You can see how it is being used in -print-after-all and -pass-times implementation followups or in the unittests.

Alternative solution would be to introduce PassBuilder::getPassInstrumentationCallbacks() method and use it for registration purposes, but I did not really like that (and yes, getPassInstrumentationCallbacks should have been dropped).

unittests/IR/PassBuilderCallbacksTest.cpp
268–272 ↗(On Diff #165518)

To be honest, my gmock voodoo is weak at best.
I was under impression that my InstrumentedSkippedPasses subtests check exactly for the routines not being called. Did I fail to test that?
(I will go read about NiceMock and see what I can do here)

283–284 ↗(On Diff #165518)

Do you mean a good matcher for llvm::Any or a good generic unwrapper from llvm::Any into IRUnit?
Since llvm::Any is just "any", I find it hard to imagine a good generic matcher for it...

423–424 ↗(On Diff #165518)

register the callbacks first

What about instrumentation that plugins want to install?
RegisterPassBuilderCallbacks does not have access to anything else except PassBuilder.

pre-registers with lambdas that delegate to the mock

Hmm... here again my weak gtest/gmock knowledge fails me.
I dont see how can I pass control to fixture constructor from mock side inside , say, TEST_F(ModuleCallbacksTest, InstrumentedPasses) - isnt it in action only when Test object (and its PassBuilder member) have already been constructed?
Care to drop a quick sketch of this idea?

463–464 ↗(On Diff #165518)

Ok, will try the sequence thing.
And about how many calls - I thought EXPECT_CALL by default means Times(1)?

philip.pfaffe added inline comments.Sep 15 2018, 8:44 AM
include/llvm/Passes/PassBuilder.h
564–568 ↗(On Diff #165518)

For plugins I'd really like to retain some means of accessing the Callbacks registry through PassBuilder. To keep it off the PassBuilder API, maybe we should pass it in the plugin APIs RegisterPassBuilderCallbacks instead? Could be done in a followup change.

vsk added a subscriber: vsk.Sep 15 2018, 11:06 AM

addressing most of the comments except getAnalysisResults.

fedor.sergeev edited the summary of this revision. (Show Details)Sep 17 2018, 1:10 PM
fedor.sergeev marked 23 inline comments as done.Sep 17 2018, 1:31 PM
fedor.sergeev added inline comments.
unittests/IR/PassBuilderCallbacksTest.cpp
463–464 ↗(On Diff #165518)

Did the sequencing change, still leaving EXPECT_CALL w/o Times(1) for brevity.

chandlerc added inline comments.Sep 18 2018, 1:50 AM
include/llvm/IR/PassInstrumentation.h
17 ↗(On Diff #165518)

"light" i think is still less clear than "clear".

85–91 ↗(On Diff #165807)

My suggestion was to go a step further, and have these be template methods that can accept an arbitrary callable (IE, a template).

If they get called with an existing unique_function, then it'll work like what you have here, but if they get called with a lambda (common case), they will construct the unique_function in place in the vector from the lambda.

103–105 ↗(On Diff #165807)

I would sink this comment to the constructor, as that is the public API that requires the lifetime. This is just the implementation detail of it.

120 ↗(On Diff #165807)

Naming conventions: ShouldRun.

include/llvm/IR/PassManager.h
568–571 ↗(On Diff #165518)

I agree that it is best if the instrumentation stuff can be in its own header.

what seems good to extract to a separate header is the PassInstrumentationAnalysis. I'm not suggesting you do that in this patch, just leave a FIXME.

include/llvm/Passes/PassBuilder.h
564–568 ↗(On Diff #165518)

I'd prefer to sort out what makes sense for plugins in a separate patch.

I think it is better (until we know what plugins actually need) to simply have users do their registration on the PassInstrumentationCallbacks object they give to the PassBuilder. For example, we don't expose any hooks to manipulate the TargetMachine from the PassBuilder -- instead, clients set it up directly themselves.

unittests/IR/PassBuilderCallbacksTest.cpp
423–424 ↗(On Diff #165518)

The callbacks object isn't *copied* into the PassBuilder -- you can leave it as a member of the fixture and directly register things onto it?

As I mention above -- I would suggest we sort out the good plugin API when we're actually building the plugin API. Even if it means we end up changing this code because it becomes easier to write it in a different way, that seems OK. Because then, we will *actually* have plugin stuff to look at, rather than hypothesizing how it might look.

463–464 ↗(On Diff #165518)

So, this EXPECT_CALL does the .Times(1), but once that expectation is saturated, there is still another expectation from the constructor that can accept as many calls as you make. So I would expect this to not fail if you call it *too many* times, only if you call it *too few*.

fedor.sergeev marked an inline comment as done.

addressing most of the comments - cleanup, unittests etc
(except getAnalysisResult, for which see D52169).

fedor.sergeev edited the summary of this revision. (Show Details)Sep 18 2018, 2:07 PM
include/llvm/IR/PassInstrumentation.h
50 ↗(On Diff #166026)

This is actually fairly important. There are many passes that will cause compilation to fail if they are skipped.

Maybe instead of having the pass managers not run the pass, you could add an argument to the run function that indicates the pass should be skipped if possible? That feels fairly clunky. The alternative is to have some way for the pass managers to detect that the passes cannot be skipped, and I suspect we don't want to do that either.

philip.pfaffe added inline comments.Sep 18 2018, 2:35 PM
include/llvm/IR/PassInstrumentation.h
50 ↗(On Diff #166026)

Passes shouldn't be able to control this I think. That burden should be on the instrumentation.

fedor.sergeev marked 9 inline comments as done.Sep 18 2018, 2:46 PM
fedor.sergeev added inline comments.
include/llvm/IR/PassInstrumentation.h
50 ↗(On Diff #166026)

This is marked as TODO :)
There are ways to allow passes opt-in/out of pass skipping, its just not a focus here.
After current series of patches will be landed I will start approaching OptBisect and then we will beat this dead horse till it is fully dead.

include/llvm/IR/PassInstrumentation.h
50 ↗(On Diff #166026)

But the instrumentation can't make this decision without having explicit knowledge about the pass. The pass itself is really the only thing that knows whether or not it can be skipped.

include/llvm/IR/PassInstrumentation.h
50 ↗(On Diff #166026)

That's fair. It doesn't need to be solved now, but I think that until it is solved the default behavior should be to not skip any passes.

fedor.sergeev added inline comments.Sep 18 2018, 2:56 PM
include/llvm/IR/PassInstrumentation.h
50 ↗(On Diff #166026)

There are still uses to the skip functionality even if it does not result in successful compilation.
Say, we downstream have a patch that gracefully stops compilation when it exceeds some resource limit,
and there we do not actually care about a successful compilation but just in graceful abort that does not leaks resources.

In this patch ability to skip the pass is more or less just a proof of concept.
Instrumentations suggested in dependent patches (-print-after-all and -time-passes) do not use skipping ability either.

I would say - if it raises questions then I'm glad it is written this way :)

include/llvm/IR/PassInstrumentation.h
50 ↗(On Diff #166026)

OK. I can live with it like this for now.

By the way, since I finally got around to commenting, I should say that I really like this design. Thanks for the work!

chandlerc accepted this revision.Sep 18 2018, 4:31 PM

LGTM (provided Philip is still happy here, I know I asked to remove his plugin hooks).

I think this is good to go in. I want to keep pushing on the tuple based analysis results stuff, but I'm happy to do that in the follow-up change. I've left some much more minor comments below. If they make sense, feel free to land this with them addressed and we'll get to the follow-ups. =D

Thanks again *so much* for the huge effort working on this. I know it was really complex, and I'm really happy with the result.

include/llvm/IR/PassInstrumentation.h
127 ↗(On Diff #166026)

Slightly stale comment re. PassID.

unittests/IR/PassBuilderCallbacksTest.cpp
324–330 ↗(On Diff #166026)

Do you just want to do this in the constructor?

332–343 ↗(On Diff #166026)

This is nice!

This revision is now accepted and ready to land.Sep 18 2018, 4:31 PM
philip.pfaffe accepted this revision.Sep 19 2018, 12:42 AM

So I'd still prefer a generic free getAnalysisResult, plus specializations for the non-simple cases, e.g. Loop and CGSCC. That way there's less surprises should there ever be a 'weird' AnalysisManager out there using a different argument mapping. Happy to do this in a followup though!

Thank you again for bearing through this lengthy review process!

fedor.sergeev added inline comments.Sep 19 2018, 3:54 AM
unittests/IR/PassBuilderCallbacksTest.cpp
324–330 ↗(On Diff #166026)

No, I dont.
I want to skip registering any callbacks in tests that do not explicitly enable pass instrumentation.

This revision was automatically updated to reflect the committed changes.
fedor.sergeev reopened this revision.Sep 19 2018, 11:28 AM

Was reverted because of massive buildbot failures - all failing to compile PassBuilderCallbacks unit-test, like this:

/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/type_traits:933:14: error: base class has incomplete type
    : public is_constructible<_Tp, const _Tp&>
      ~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/type_traits:939:14: note: in instantiation of template class 'std::__is_copy_constructible_impl<testing::Matcher<llvm::Any>, true>' requested here
    : public __is_copy_constructible_impl<_Tp>
             ^
/home/llvmbb/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/include/llvm/ADT/STLExtras.h:69:49: note: in instantiation of template class 'std::is_copy_constructible<testing::Matcher<llvm::Any> >' requested here
template <typename B1> struct conjunction<B1> : B1 {};
This revision is now accepted and ready to land.Sep 19 2018, 11:28 AM

adding a hack to PassBuilderCallbacks unittest that allows
to use llvm::Any in mocks.

This revision was automatically updated to reflect the committed changes.
fedor.sergeev reopened this revision.Sep 20 2018, 8:05 AM

Sanitizer buildbots failed on CGSCC tests due to improper getName handling (StringRef on a temporary std::string cant fly well)

This revision is now accepted and ready to land.Sep 20 2018, 8:05 AM

changed getName helper in unittests to return std::string instead of StringRef
to avoid addressing temporaries after their lifetime end.Z

This revision was automatically updated to reflect the committed changes.