This is an archive of the discontinued LLVM Phabricator instance.

[Timers] Use the pass argument name for JSON keys in time-passes
ClosedPublic

Authored by thegameg on Jun 12 2018, 5:11 PM.

Details

Summary

When using clang --save-stats -mllvm -time-passes, both timers and stats end up in the same json file.

We could end up with things like:

{
  "asm-printer.EmittedInsts": 1,
  "time.pass.Virtual Register Map.wall": 2.9015541076660156e-04,
  "time.pass.Virtual Register Map.user": 2.0500000000000379e-04,
  "time.pass.Virtual Register Map.sys": 8.5000000000001741e-05,
}

This patch makes use of the pass argument name (if available) in the JSON key to end up with things like:

{
  "asm-printer.EmittedInsts": 1,
  "time.pass.virtregmap.wall": 2.9015541076660156e-04,
  "time.pass.virtregmap.user": 2.0500000000000379e-04,
  "time.pass.virtregmap.sys": 8.5000000000001741e-05,
}

This also helps avoiding to write another JSON printer to handle all the cases that we could have in our pass names.

Diff Detail

Event Timeline

thegameg created this revision.Jun 12 2018, 5:11 PM
thegameg edited reviewers, added: paquette; removed: jpaquette.Jun 12 2018, 5:12 PM

I guess this makes sense.
But what about new pass manager?

Adding some reviewers from the differential (D25583) that added the LHS of the diff.

This would make much more sense IMO. Especially for passes with long names. (For example, I'd much rather see "patchable-function" than "Implement the 'patchable-function' attribute" in the JSON)

I guess this makes sense.
But what about new pass manager?

A quick grep shows that Timers are currently only used in the legacy PM.

Is timing info implemented in the new PM yet? If not, I think it would make sense to just move forward with this; the change is pretty simple, and I can't really see it blocking or increasing the work that'd have to go into a new PM implementation.

Other than the new PM question, this LGTM.

This would make much more sense IMO. Especially for passes with long names. (For example, I'd much rather see "patchable-function" than "Implement the 'patchable-function' attribute" in the JSON)

I guess this makes sense.
But what about new pass manager?

A quick grep shows that Timers are currently only used in the legacy PM.

Is timing info implemented in the new PM yet? If not, I think it would make sense to just move forward with this; the change is pretty simple, and I can't really see it blocking or increasing the work that'd have to go into a new PM implementation.

No idea.
I'm simply noting that this only touches only one of the two pm's.
If the new one does not have this functionality at all, then yes, this is good as-is.

Other than the new PM question, this LGTM.

Same.

@chandlerc @jlebar can you confirm that the new PM doesn't have timer support?

MatzeB accepted this revision.Jun 13 2018, 11:31 AM

@chandlerc @jlebar can you confirm that the new PM doesn't have timer support?

I'm pretty sure I only added the timer->statistics support for the "legacy" pass manager.

LGTM.

I suspect there aren't too many people tracking those stats yet so renaming should be fine.

(I also wish pass names and argument names would just be the same, but this is fine for the status quo).

This revision is now accepted and ready to land.Jun 13 2018, 11:31 AM
This revision was automatically updated to reflect the committed changes.

FYI, this caused a regression: http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/49533/testReport/junit/LLVM/Other/statistic_ll/

/Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm/test/Other/statistic.ll:11:17: error: expected string not found in input
; JSONTIME-DAG: "time.pass.Remove redundant instructions.wall"
                ^
<stdin>:2:2: note: scanning from here
 "instsimplify.NumSimplified": 1,
 ^

Reverted in r334651.