[tools][llc] Add --save-stats option#163967
Conversation
tomershafir
force-pushed
the
llc-save-stats
branch
from
6d342b5 to
ba34d04
Compare
This patch adds a Clang-compatible `--save-stats` option, to provide an easy to use way to save LLVM statistics files when working with llc on the backend. Like on Clang, one can specify `--save-stats`, `--save-stats=cwd`, and `--save-stats=obj` with the same semantics and JSON format. The implementation uses 2 methods `MaybeEnableStats` and `MaybeSaveStats` called before and after `compileModule` respectively that externally own the statistics related logic, while `compileModule` is now required to return the resolved output filename via an output param. Note: like on Clang, the pre-existing `--stats` option is not affected.
tomershafir
force-pushed
the
llc-save-stats
branch
from
ba34d04 to
56a0157
Compare
fhahn
left a comment
fhahn
left a comment
There was a problem hiding this comment.
If we decide to add this flag, I think it should be consistent across all tools that have a --stats option.
|
Feels somewhat odd that |
|
I planned to add it to |
|
I see also |
|
Given |
|
Would a simpler version work that just takes a filename instead of all the logic with [1] I think the logic kinda makes sense for the clang driver which naturally does this sort of transformation and filename construction all the time; but for these small developers tools within LLVM it seems overkill... |
|
Reading the existing code you can already do: (Admittedly the existing flags are kinda hard to discover and don't have great names; but oh well...) |
|
Agree. In my downstream project I have to do the extra substr to get the JSON body.
Adding this option to opt is enough. I don't see the value of putting the logic in libSupport. A bit of code duplication should be acceptable. |
Not sure what you are getting at here; there's only timers (
When using I'm not convinced the code duplication is that good an idea, so -1 from me. But not gonna block this if people think that |
Agree. I think llc and opt are enough. Adding it to libSupport can complicate things. It would also require migrating the Clang options there and couple Clang to these options.
I think |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
tomershafir
force-pushed
the
llc-save-stats
branch
from
8b69713 to
860c71a
Compare
tomershafir
force-pushed
the
llc-save-stats
branch
from
860c71a to
a377f76
Compare
fhahn
left a comment
fhahn
left a comment
There was a problem hiding this comment.
Just to mention another possible alternative, @tobias-stadler has been working on emitting statistics for each function as a remark, which would allow for higher granularity.
I think this can be viewed as an implementation detail here, which would still require LLVM statistics semantics as described here on top, so I dont think it should block this enhancement. |
|
Since there is no strong resistance or further inputs, going to merge this soon |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/39698 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/27/builds/18682 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/10/builds/16966 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/3/builds/24601 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/23797 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/26069 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/54/builds/14490 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/33777 Here is the relevant piece of the build log for the reference |
A quick fix for llvm#163967
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/155/builds/14681 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/15392 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/13/builds/10522 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/27920 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/28060 Here is the relevant piece of the build log for the reference |
A quick fix for the CI failure introduced by #163967
…7218) A quick fix for the CI failure introduced by llvm/llvm-project#163967
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/38/builds/6256 Here is the relevant piece of the build log for the reference |
This patch adds a Clang-compatible --save-stats option to opt, to provide an easy to use way to save LLVM statistics files when working with opt on the middle end. This is a follow up on the addition to `llc`: llvm#163967 Like on Clang, one can specify --save-stats, --save-stats=cwd, and --save-stats=obj with the same semantics and JSON format. The pre-existing --stats option is not affected. The implementation extracts the flag and its methods into the common `CodeGen/CommandFlags` as `LLVM_ABI`, using a new registration class to conservatively enable opt-in rather than let all tools take it. Its only needed for llc and opt for now. Then it refactors llc and adds support for opt.
This patch adds a Clang-compatible --save-stats option to opt, to provide an easy to use way to save LLVM statistics files when working with opt on the middle end. This is a follow up on the addition to `llc`: llvm#163967 Like on Clang, one can specify --save-stats, --save-stats=cwd, and --save-stats=obj with the same semantics and JSON format. The pre-existing --stats option is not affected. The implementation extracts the flag and its methods into the common `CodeGen/CommandFlags` as `LLVM_ABI`, using a new registration class to conservatively enable opt-in rather than let all tools take it. Its only needed for llc and opt for now. Then it refactors llc and adds support for opt.
This patch adds a Clang-compatible --save-stats option to opt, to provide an easy to use way to save LLVM statistics files when working with opt on the middle end. This is a follow up on the addition to `llc`: llvm#163967 Like on Clang, one can specify --save-stats, --save-stats=cwd, and --save-stats=obj with the same semantics and JSON format. The pre-existing --stats option is not affected. The implementation extracts the flag and its methods into the common `CodeGen/CommandFlags` as `LLVM_ABI`, using a new registration class to conservatively enable opt-in rather than let all tools take it. Its only needed for llc and opt for now. Then it refactors llc and adds support for opt.
This patch adds a Clang-compatible --save-stats option to opt, to provide an easy to use way to save LLVM statistics files when working with opt on the middle end. This is a follow up on the addition to `llc`: #163967 Like on Clang, one can specify --save-stats, --save-stats=cwd, and --save-stats=obj with the same semantics and JSON format. The pre-existing --stats option is not affected. The implementation extracts the flag and its methods into the common `CodeGen/CommandFlags` as `LLVM_ABI`, using a new registration class to conservatively enable opt-in rather than let all tools take it. Its only needed for llc and opt for now. Then it refactors llc and adds support for opt.
This patch adds a Clang-compatible --save-stats option to opt, to provide an easy to use way to save LLVM statistics files when working with opt on the middle end. This is a follow up on the addition to `llc`: llvm/llvm-project#163967 Like on Clang, one can specify --save-stats, --save-stats=cwd, and --save-stats=obj with the same semantics and JSON format. The pre-existing --stats option is not affected. The implementation extracts the flag and its methods into the common `CodeGen/CommandFlags` as `LLVM_ABI`, using a new registration class to conservatively enable opt-in rather than let all tools take it. Its only needed for llc and opt for now. Then it refactors llc and adds support for opt.
This patch adds a Clang-compatible `--save-stats` option, to provide an easy to use way to save LLVM statistics files when working with llc on the backend. Like on Clang, one can specify `--save-stats`, `--save-stats=cwd`, and `--save-stats=obj` with the same semantics and JSON format. The implementation uses 2 methods `MaybeEnableStats` and `MaybeSaveStats` called before and after `compileModule` respectively that externally own the statistics related logic, while `compileModule` is now required to return the resolved output filename via an output param. Note: like on Clang, the pre-existing `--stats` option is not affected. (chery-pick 96a5289)
A quick fix for the CI failure introduced by llvm#163967 (cherry-pick b15e220)
This patch adds a Clang-compatible --save-stats option to opt, to provide an easy to use way to save LLVM statistics files when working with opt on the middle end. This is a follow up on the addition to `llc`: llvm#163967 Like on Clang, one can specify --save-stats, --save-stats=cwd, and --save-stats=obj with the same semantics and JSON format. The pre-existing --stats option is not affected. The implementation extracts the flag and its methods into the common `CodeGen/CommandFlags` as `LLVM_ABI`, using a new registration class to conservatively enable opt-in rather than let all tools take it. Its only needed for llc and opt for now. Then it refactors llc and adds support for opt. (cherry-pick 35ffe10)
6 participants
This patch adds a Clang-compatible
--save-statsoption, to provide an easy to use way to save LLVM statistics files when working with llc on the backend.Like on Clang, one can specify
--save-stats,--save-stats=cwd, and--save-stats=objwith the same semantics and JSON format.The implementation uses 2 methods
MaybeEnableStatsandMaybeSaveStatscalled before and aftercompileModulerespectively that externally own the statistics related logic, whilecompileModuleis now required to return the resolved output filename via an output param.Note: like on Clang, the pre-existing
--statsoption is not affected.