Page MenuHomePhabricator

[New PM][PassTiming] implement -time-passes for the new pass manager

Authored by fedor.sergeev on Aug 26 2018, 5:12 PM.



Enable time-passes functionality through PassInstrumentation callbacks
for passes and analyses.

TimePassesHandler class keeps all the callbacks as well as the state needed.
PassTiming class manages timer group and a mapping between PassID and Timer.

Parts of the fix that might be somewhat unobvious:

  • mapping of passes into Timer (TimingData) can not be done per-instance. PassID name provided into the callback is common for all the pass invocations. Thus the only way to get a timing with reasonable granularity is to collect timing data per pass invocation, getting a new timer for each BeforePass. Hence the key for TimingData uses a pair of <StringRef/unsigned count> to uniquely identify a pass invocation.
  • consequently, this new-pass-manager implementation performs no aggregation of timing data, reporting timings for each pass invocation separately. In that it differs from legacy-pass-manager time-passes implementation that reports timing data aggregated per pass instance.
  • pass managers and adaptors are not tracked, similar to how pass managers are not tracked in legacy time-passes.
  • TimePassesHandler object keeps a stack of timers that are active, each BeforePass pushes the new timer on stack, each AfterPass pops active timer from stack and stops it.

Diff Detail


Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
fedor.sergeev edited the summary of this revision. (Show Details)Sep 7 2018, 1:24 PM


rebasing on top of new changes and removing pass-managers tracking.
Now it collects timing data per-pass-invocation.

fedor.sergeev edited the summary of this revision. (Show Details)Sep 18 2018, 1:55 PM

separated legacy and newpm implementations, introducing TimePassesHelper object that
manages callbacks and keeps timing data. That in particular allows to avoid
singleton TimeInfo and mutex - all the weird stuff initially inherited from
a shared implementation.

fedor.sergeev edited the summary of this revision. (Show Details)Sep 20 2018, 3:46 PM

Some inline comments.

23 ↗(On Diff #166375)

You could forward declare PassInstrumentationCallbacks instead.

85 ↗(On Diff #166375)

What if someone wants a legacy StringRef PassTimingInfo. This should just be a new standalone type, especially since PassTimingInfo is somewhat of a template "interface" that your specialization doesn't fulfill anymore.

104 ↗(On Diff #166375)

The implementation doesn't reset.

127 ↗(On Diff #166375)

Why is this explicit when it doesn't do anything?

150 ↗(On Diff #166375)

A private static can just be a free static function.

154 ↗(On Diff #166375)

Why doesn't this contain the object directly?

60 ↗(On Diff #166375)

Is this include actually used?

adding TimePasses to StandardInstrumentations, cleaning up a bit.

rebased on top of D52356 cleanup

fedor.sergeev marked 4 inline comments as done.Sep 21 2018, 6:07 AM
fedor.sergeev added inline comments.
104 ↗(On Diff #166375)

It actually does. This is a weird part of underneath TimeGroup::print() functionality.

127 ↗(On Diff #166375)

Just to add a comment! :)

154 ↗(On Diff #166375)

To avoid creating TimingInfo (its TimeGroup member in particular) if -time-passes is not enabled.

I like how simple this is becoming!

Comments inline.

44 ↗(On Diff #166467)

The comment should instead explain what this class does and how it's intended to be used.

47 ↗(On Diff #166467)

Explain the semantics of the ID.

51 ↗(On Diff #166467)

PassTiming owns all Timer instances, doesn't it? Store a value instead of a new'ed pointer.

52 ↗(On Diff #166467)

Comments on extra line instead?

78 ↗(On Diff #166467)

Class comments?

79 ↗(On Diff #166467)

This goes below, to the rest of the private members.

154 ↗(On Diff #166375)

But why? TimerGroup's initialization doesn't really do anything important, and you pay with complexity, an allocation, and all the assertions below.

22 ↗(On Diff #166467)

Superfluous include.

145 ↗(On Diff #166467)

Extra whitespace.

60 ↗(On Diff #166375)

Marked as done but the include is still there

111 ↗(On Diff #166467)

I don't think these comments here actually add anything.

addressing review comments. In particular significantly simplifying memory management,
getting rid of extra allocations.

fedor.sergeev marked 11 inline comments as done.Sep 25 2018, 6:16 AM
fedor.sergeev added inline comments.
111 ↗(On Diff #166467)

if you grep the sources for the name of the option you will find this.
Other than that - yes, it does not add anything to the code readability.

philip.pfaffe added inline comments.Sep 26 2018, 1:57 AM
50 ↗(On Diff #166863)

This needn't even be public.

52 ↗(On Diff #166863)

Try to minimize the number of visibility edges. I.e. reorder the members, public first, private second.

71 ↗(On Diff #166863)

Instead of this empty thing that's just a comment, actively clear() the TimingData here. Then it's explicit what is supposed to happen here, and you don't rely on destruction order of the class members.

addressing comments

fedor.sergeev marked 4 inline comments as done.Sep 26 2018, 5:23 AM
philip.pfaffe accepted this revision.Sep 27 2018, 2:48 AM

LGTM! Some final feedback by @chandlerc would be good though before landing :)

This revision is now accepted and ready to land.Sep 27 2018, 2:48 AM
fedor.sergeev added 1 blocking reviewer(s): chandlerc.Oct 1 2018, 5:07 AM
This revision now requires review to proceed.Oct 1 2018, 5:07 AM

running time-passes on longer pipelines revealed a problem with
making Timer managed by DenseMap in TimingData - we cant copy the Timer
on map expansion. Thus reverting back to keeping Timer* in that map
and new/deleting the instances.

Added to time-passes test an invocation that runs -O2 to expose this
situation (that particular invocation was failing in Timer's copy constructor
w/o the fix).

Some more minor code comments inline...

While I'm fine for it to arrive in a follow-up patch, I do think we'll want a mechanism to aggregate by pass name, and I suspect that should even be the default, with the per-invocation timings available by passing more detailed flags. (In many cases, the per-invocation will be *massive* due to the number of functions in the module.)

49 ↗(On Diff #167928)

Maybe PassInvocationID? That makes the name more directly explain what it does.

55 ↗(On Diff #167928)

Why not std::unique_ptr<Timer>?

71 ↗(On Diff #167928)

I would suggest using idiomatic STL APIs here rather than the (quite odd) APIs from DenseMap. Common pattern is:

for (auto &KV : Timingdata)
  delete KV.second;
94 ↗(On Diff #167928)

This comment seems more like a fragment...

114 ↗(On Diff #167928)

Don't think this comment really helps.

120 ↗(On Diff #167928)

What is the advantage of having two separate types here? (Not a rhetorical question, just isn't obvious to me on reading the code.)

111 ↗(On Diff #166467)

Maybe if they were full prose it would be more clear? As it is, these are I think mostly helpful given context that you have implicitly in your head. I suspect expanding them into prose will either make them both clear and useful to readers, or will make it more obvious that they are just repeating what the code already clearly states it is doing.

fedor.sergeev added inline comments.Oct 4 2018, 2:32 AM
120 ↗(On Diff #167928)

TimingInfo is a data that we collect and then report.
And TimerStack is a helper for the instrumentation to keep the context of pass managers as they are being executed as of this moment.
At the end of collection TimerStack is empty.

I feel it somewhat convenient to mentally separate them.
But I dont have any strong feelings about that.

as for the per-pass-name aggregation - I was going to supply a follow-up patch that changes -time-passes from boolean option to -time-passes=<aggregation-type>.
Perhaps aggregation-type should be 'aggregate' by default, and 'invocations' providing per-invocation info.
Yet I dont want to just remove the possibility of getting a more fine-grained data that we have in this patch.

as for the per-pass-name aggregation - I was going to supply a follow-up patch that changes -time-passes from boolean option to -time-passes=<aggregation-type>.
Perhaps aggregation-type should be 'aggregate' by default, and 'invocations' providing per-invocation info.
Yet I dont want to just remove the possibility of getting a more fine-grained data that we have in this patch.

Yeah, i definitely like having the per-invocation information. I'm just suggesting that (eventually, happy for this to come in follow-ups) we'll probably want the default to be aggregated a bit.

addressing comments - moving to unique_ptr, comments cleanup

fedor.sergeev marked 7 inline comments as done.Oct 4 2018, 9:04 AM
fedor.sergeev added inline comments.
111 ↗(On Diff #166467)

Oh, well, perhaps I better strip them off... nothing really important "implicitly in my head" :)

chandlerc accepted this revision.Oct 5 2018, 3:18 AM

Somewhat minor code simplification below. If it makes sense, feel free to land. If it doesn't honestly, feel free to explain why not and land. Even if I convince you, it's super simple to land as a follow-up. =]

120 ↗(On Diff #167928)

I'm just suggesting to inline the PassTiming class into this one. Not seeing much utility in it being two types as opposed to one.

This revision is now accepted and ready to land.Oct 5 2018, 3:18 AM
fedor.sergeev marked 3 inline comments as done.Oct 5 2018, 11:19 AM
fedor.sergeev added inline comments.
120 ↗(On Diff #167928)

Hmm... i like the suggestion.
Iit appears that I was so much obsessed about keeping implementations of PassTiming in line that I just did not consider this. :-/

This revision was automatically updated to reflect the committed changes.
fedor.sergeev marked an inline comment as done.