This is an archive of the discontinued LLVM Phabricator instance.

[NewPM][TimePasses] allow -time-passes reporting into a custom stream
ClosedPublic

Authored by fedor.sergeev on Mar 14 2019, 7:55 AM.

Details

Summary

TimePassesHandler object (implementation of time-passes for new pass manager)
gains ability to report into a stream customizable per-instance (per pipeline).

Intended use is to specify separate time-passes output stream per each compilation,
setting up TimePasses member of StandardInstrumentation during PassBuilder setup.
That allows to get independent non-overlapping pass-times reports for parallel
independent compilations (in JIT-like setups).

By default it still puts timing reports into the info-output-file stream
(created by CreateInfoOutputFile every time report is requested).

Unit-test added for non-default case, and it also allowed to discover that print() does not work
as declared - it did not reset the timers, leading to yet another report being printed into the default stream.
Fixed print() to actually reset timers according to what was declared in print's comments before.

Diff Detail

Event Timeline

fedor.sergeev created this revision.Mar 14 2019, 7:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 7:55 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
fedor.sergeev retitled this revision from [TimePasses] allow -time-passes reporting into a custom stream to [NewPM][TimePasses] allow -time-passes reporting into a custom stream.Mar 14 2019, 8:00 AM

As per discussion with @philip.pfaffe - owning output stream is rather strange for TimePasses object.
This ownership idea came from CreateInfoOutputFile design, as it returns new stream each time.

Now making OutStream being just a non-owning pointer and handling ownership ouside of TimePasses
(in NewPMDriver).

fedor.sergeev edited the summary of this revision. (Show Details)Mar 14 2019, 8:24 AM
fedor.sergeev planned changes to this revision.Mar 14 2019, 8:38 AM

outstream in TimePasses is set when enabling, so now it is not possible
to enable and have outstream == nullptr.

Why not default to CreateInfoOutputFile inside of TimePassesHandler if OutputStream is unset? And then have a setStream(raw_ostream&) method to change it.

Why not default to CreateInfoOutputFile inside of TimePassesHandler if OutputStream is unset? And then have a setStream(raw_ostream&) method to change it.

That is purely a chicken/egg issue.
CreateInfoOutputFile creates a new stream and unless we retake the ownership it will be destroyed.

So there are two approaches to set it for TimePasses by default:

  • own it in TimePassesHandler (my initial implementation)
  • pass the non-owning pointer into the default constructor (requires passing it to StandardInstrumentation first, so not ideal either)

CreateInfoOutputFile creates a new stream and unless we retake the ownership it will be destroyed.

True, but that's what it is doing right now. Do you see reason to change that?

CreateInfoOutputFile creates a new stream and unless we retake the ownership it will be destroyed.

True, but that's what it is doing right now. Do you see reason to change that?

Okey, let me enumerate all the possible solutions that I see:

  1. own stream in TimePassesHandler (my initial implementation)
  2. create TimePasses disabled/no-stream, enable from NewPMDriver (current state), own by NewPMDriver
  3. pass the stream to StandardInstrumentation constructor, pass through into TimePassesHandler constructor, own by NewPMDriver
  4. change behavior of CreateInfoOutputFile and make it handle the ownership itself

I cant do 4. because that will change -stats behavior as well as -time-passes behavior with old-pm.
We have already decided that 1. is no-go.
So we are left with either 2. or 3.

And 3. requires StandardInstrumentation to decide on how to pass this stream around.
What if PrintIR will be changed to take the stream as well? Should StandardInstrumentation decide to pass the same stream there?
If no then should we add one more stream parameter to StandardInstrumentation constructor?
It does not seem to scale that much....

fedor.sergeev planned changes to this revision.Mar 14 2019, 11:11 AM

Okey, a proper solution here is to get back to not setting the stream by default and restort to old CreateInfoOutputFile-just-for-printing scheme.
This way we do not need to play with ownership of that info-output-file stream.

The only piece that needs work is unittesting for the new interface, since it will not be utilized in opt/NewPMDriver.

handling default case the same way as it was before;
adding unit-test for non-default case

fedor.sergeev edited the summary of this revision. (Show Details)Mar 14 2019, 2:16 PM

Couple of nits inline, otherwise looks great!

lib/IR/PassTimingInfo.cpp
190

I think this shouldn't be an error. If someone changes the stream for a disabled timer, so what?

195

Calling this now flushes all timer data. Is that worth a comment?

198

Isn't that a use-after-free? Is the stream guaranteed to survive the call to operator*?

tools/opt/NewPMDriver.cpp
18

Unrelated change.

262

Unrelated change.

unittests/IR/TimePassesTest.cpp
18 ↗(On Diff #190724)

Are the gmock and Error includes actually used?

31 ↗(On Diff #190724)

Do you need any of these?

58 ↗(On Diff #190724)

Maybe leave a comment why this is here.

fedor.sergeev marked 3 inline comments as done.Mar 15 2019, 6:16 AM

Thanks for review!

lib/IR/PassTimingInfo.cpp
190

There is no way to reenable the timer. So I thought setting the stream for disabled timer means some confusion.... I'm not sure about this

195

The comment is there in the header. It was a "declared" functionality both in legacy and new interfaces, but it actually was never implemented.
Catched this when writing the unit-test.
Btw, I'm adding a check for this into the unit-test, and also preparing a separate patch that adds this flushing for legacy.

198

Temporaries are cleaned up at the end of a full-expression, in this case - afteri a TG.print call.

unittests/IR/TimePassesTest.cpp
18 ↗(On Diff #190724)

yeah, this was copied from some other test, will clean it out.

58 ↗(On Diff #190724)

will add a bit more code here, with comments...

test cleanup - removing unneeded stuff, adding a few more checks, adding comments

fedor.sergeev edited the summary of this revision. (Show Details)Mar 15 2019, 6:56 AM
fedor.sergeev marked 6 inline comments as done.Mar 15 2019, 7:20 AM

removing unneeded assert

fedor.sergeev marked 2 inline comments as done.Mar 15 2019, 11:40 AM
philip.pfaffe accepted this revision.Mar 15 2019, 11:46 AM

Looks good!

This revision is now accepted and ready to land.Mar 15 2019, 11:46 AM

Thanks for yet another thorough review!

This revision was automatically updated to reflect the committed changes.