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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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? |
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). |
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. |
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.