Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

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

Authored by fedor.sergeev on Mar 15 2019, 8:02 AM.



As a followup to newpm -time-passes fix (D59366), now adding a similar
functionality to legacy time-passes.

Enhancing llvm::reportAndResetTimings to accept an optional stream
for reporting output. By default it still reports into info-output-file.

Also fixing to actually reset after printing as declared.

Diff Detail


Event Timeline

fedor.sergeev created this revision.Mar 15 2019, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 15 2019, 8:02 AM
Herald added a subscriber: kristina. · View Herald Transcript

Just two inline nits.

84 ↗(On Diff #190826)

You have a lot of namespace switches here. If you reorder slightly, that's unnecessary.

moved legacy test code around to remove the need for additional namespace manipulations

philip.pfaffe added inline comments.Mar 20 2019, 4:51 AM
32 ↗(On Diff #191476)

Sorry, this second nit got lost: Is it clear from context what info-output-file is? Otherwise reference CreateInfoOutputFile() instead.

29 ↗(On Diff #191476)

Is this one still necessary?

fedor.sergeev marked 2 inline comments as done.Mar 20 2019, 5:22 AM
fedor.sergeev added inline comments.
32 ↗(On Diff #191476)

Well, both of these would require a search through sources if you do not know what it is.
info-output-file is a command-line level control, CreateInfoOutputFile is an API control.
I really do not have any preferences here.

29 ↗(On Diff #191476)

well... as I see it is quite common through the unittests to use anonymous namespaces to hide test stuff
(I gather to avoid any clashes with LLVM interfaces).
Say, LegacyPassManagerTest does pretty much the same.
Its just that weird requirement of INITIALIZE_PASS for legacy passes to be inside namespace llvm that requires
extra namespace busywork.

I'm open for any suggestions here.

fedor.sergeev marked 2 inline comments as not done.Mar 20 2019, 5:23 AM
fedor.sergeev added inline comments.Mar 20 2019, 5:31 AM
32 ↗(On Diff #191476)

Will it be more clear if I say "By default it uses file stream specified by -info-output-file" ?

philip.pfaffe added inline comments.Mar 20 2019, 10:48 AM
32 ↗(On Diff #191476)

Technically that's not correct though, per default it uses CreateInfoOutputFile as far as the API is concerned. That that then reads the command line option is still one more hop.

29 ↗(On Diff #191476)

Sure, you're opening another anonymous namespace 30 lines down though.

fedor.sergeev added inline comments.Mar 20 2019, 1:16 PM
32 ↗(On Diff #191476)


29 ↗(On Diff #191476)

*that* is a completely different anonymous namespace, which is not nested into llvm.

this one:
namespace llvm {

namespace {
   ... passes here

is a namespace for Passes, which are required to be llvm::something, otherwise INITIALIZE_PASS does not seem to work.

and the next one:
namespace {

... tests here

is a namespace for tests, which can be whatever you want.
I can remove the latter and put everything (passes and tests) inside the former (nested inside llvm::) one.
I dont see it as a good solution, but I dont have strong feelings about that.

comments updated

fedor.sergeev marked 6 inline comments as done.Mar 20 2019, 2:46 PM
philip.pfaffe accepted this revision.Mar 22 2019, 5:42 AM

On final language nit, otherwise LGTM!

67 ↗(On Diff #191590)

Maybe 'into the stream created by CreateInfoOutputFile()'? Also in order to get the doxygen linking to kick in.

This revision is now accepted and ready to land.Mar 22 2019, 5:42 AM
This revision was automatically updated to reflect the committed changes.