Page MenuHomePhabricator

[New PM] Introducing PassInstrumentation framework

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



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


Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
fedor.sergeev added inline comments.Sep 14 2018, 6:34 AM
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.

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.

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) {

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?

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?

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.

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...

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.

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.

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.

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.

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).

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
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.
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
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.

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.

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.

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
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
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.
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.

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.

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
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 :)

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.

127 ↗(On Diff #166026)

Slightly stale comment re. PassID.

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
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.