This is an archive of the discontinued LLVM Phabricator instance.

Timer: Track Name and Description.
ClosedPublic

Authored by MatzeB on Oct 13 2016, 4:18 PM.

Details

Summary

Currently the "names" of timers are something like "DAG Combining after legalize types", this is more of a description than a name and unwieldy when exporting into machine readable formats like JSON. Call the current timer names descriptions and add a new name field with a short programming language identifier like name.

Diff Detail

Repository
rL LLVM

Event Timeline

MatzeB updated this revision to Diff 74595.Oct 13 2016, 4:18 PM
MatzeB retitled this revision from to Timer: Track Name and Description..
MatzeB updated this object.
MatzeB set the repository for this revision to rL LLVM.
MatzeB added a subscriber: llvm-commits.
vsk edited edge metadata.Oct 13 2016, 4:30 PM

This looks good! I only have nits. I assume clang will need some updates when you land this?

lib/IR/LegacyPassManager.cpp
452 ↗(On Diff #74595)

Bikeshed: maybe "timing_info" for the name here? (I don't have a hard '-1' to just keeping "pass".)

lib/Support/Timer.cpp
214 ↗(On Diff #74595)

getNamedRegionTimer has no coverage -- it looks safe to delete in a prep commit.

Thanks for the review, yes I have a clang patch as well.

lib/IR/LegacyPassManager.cpp
452 ↗(On Diff #74595)

This will result in names like: time.pass.Loop Versioning which I find nicer than time.timing_info.Loop Versioning, don't you think?
(This also shows that unfortunately our pass "names" have the same problem that they are more like descriptions, but as far as I see it that problem will solve itself once the new pass manager has landed which will use the class names of the passes).

lib/Support/Timer.cpp
214 ↗(On Diff #74595)

Yes that is https://reviews.llvm.org/D25582 (some bits of that are visible here as well).

vsk added a comment.Oct 13 2016, 4:53 PM

Could you either delete Timer::getName(), or audit it / upgrade its uses? There is no coverage for it either, so it can probably go along with getNamedRegionTimer.

It looks like this patch is NFCI, but it might help to have some kind of test that ensures getName() and getDescription() return something reasonable. Maybe in unittests/Support/TimerTests?

lib/IR/LegacyPassManager.cpp
452 ↗(On Diff #74595)

Sure. I had missed the 'time.' prefix.

In D25583#569843, @vsk wrote:

Could you either delete Timer::getName(), or audit it / upgrade its uses? There is no coverage for it either, so it can probably go along with getNamedRegionTimer.

Good point. Removing it produces no compile error here, so I think it is indeed unused. However being the only accessor function to this information I'd rather not delete it.

vsk accepted this revision.Oct 14 2016, 10:16 AM
vsk edited edge metadata.

Lgtm.

This revision is now accepted and ready to land.Oct 14 2016, 10:16 AM
bruno accepted this revision.Oct 14 2016, 6:53 PM
bruno edited edge metadata.

This is great Matthias. Thank for working on this.

LGTM

echristo accepted this revision.Oct 20 2016, 3:48 PM
echristo edited edge metadata.

LGTM.

-eric

This revision was automatically updated to reflect the committed changes.