This is an archive of the discontinued LLVM Phabricator instance.

Extend TimeTrace to LLVM's new pass manager
ClosedPublic

Authored by amonshiz on Feb 12 2020, 3:50 PM.

Details

Summary

With the addition of the LLD time tracing it made sense to include coverage
for LLVM's various passes. Doing so ensures that ThinLTO is also covered
with a time trace.

Before:

After:

Diff Detail

Event Timeline

amonshiz created this revision.Feb 12 2020, 3:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 12 2020, 3:50 PM
mattd added a subscriber: mattd.Feb 12 2020, 4:09 PM
Matt added a subscriber: Matt.Feb 12 2020, 6:39 PM
amonshiz updated this revision to Diff 245178.Feb 18 2020, 8:56 AM

Friendly ping for reviews on this one. I've found this to be a great tool for debugging the passes running during ThinLTO.

I've faced with objections from @philip.pfaffe trying to implement the same: D62067. Since that time, trace profiler has supported multiple threads (D71059), so I do believe this can now correctly work. I would wait for review from new pm guys.

Friendly ping! New PM guys, maybe @chandlerc or @lebedev.ri could review?

lebedev.ri resigned from this revision.Mar 4 2020, 2:07 PM

I'm very much not the right reviewer here, sorry!

rnk accepted this revision.Mar 4 2020, 4:42 PM

lgtm!

Sorry for the delay, I have been busy. I hadn't opened this up until now, but it seems straightforward, I don't think we need additional thorough review from local area code owners.

This revision is now accepted and ready to land.Mar 4 2020, 4:42 PM
rnk added a comment.Mar 6 2020, 2:16 PM

Based on perusing https://reviews.llvm.org/p/amonshiz/, I assume you need someone to push this. I'll go ahead and do that.

This revision was automatically updated to reflect the committed changes.

thank you @rnk for committing this for me!

I'm sorry for the late comment, but this should have really been done through the PassInstrumentation framework.
It looks like a classic use-case.

As an explanation why PassInstrumentation is a better choice - you do not need to manually extend all the individual pass managers,
so you never miss one. This particular change missed at least two of them: RepeatPass and DevirtSCCRepeatedPass.

I'm sorry for the late comment, but this should have really been done through the PassInstrumentation framework.
It looks like a classic use-case.

AFAII, this couldn't be done using register[Before/After]PassCallback() (like here llvm/lib/IR/PassTimingInfo.cpp) since profiler timer have to be assuredly finished if started, and that should be done directly by inserting [start/stop]TimeProfiler() in run[Before/After/Pass/Invalidated]().