Page MenuHomePhabricator

[ADT] Remove StatisticBase and make NoopStatistic empty
ClosedPublic

Authored by MaskRay on Apr 23 2021, 5:05 PM.

Details

Summary

In LLVM_ENABLE_STATS=0 builds, llvm::Statistic maps to llvm::NoopStatistic but has 3 mostly unused
pointers. GlobalOpt considers that the pointers can potentially retain allocated
objects, so GlobalOpt cannot optimize out the NoopStatistic variables (see
D69428 for more context), wasting 23KiB for stage 2 clang.

This patch makes NoopStatistic empty and thus reclaims the wasted space. The
clang size is even smaller than applying D69428 (slightly smaller in both .bss and
.text).

# this means the D69428 optimization on clang is mostly nullified by this patch. There may be cases GlobalOpt doesn't do a good job optimizing out the old NoopStatistic and we probably need to investigate
HEAD+D69428: size(.bss) = 0x0725a8
HEAD+D101211: size(.bss) = 0x072238

# bloaty - HEAD+D69428 vs HEAD+D101211
# With D101211, we also save a lot of string table space (.rodata).
    FILE SIZE        VM SIZE    
 --------------  -------------- 
  -0.0%     -32  -0.0%     -24    .eh_frame
  -0.0%    -336  [ = ]       0    .symtab
  -0.0%    -360  [ = ]       0    .strtab
  [ = ]       0  -0.2%    -880    .bss
  -0.0% -2.11Ki  -0.0% -2.11Ki    .rodata
  -0.0% -2.89Ki  -0.0% -2.89Ki    .text
  -0.0% -5.71Ki  -0.0% -5.88Ki    TOTAL

Note: LoopFuse is a disabled pass. For now this patch adds #if LLVM_ENABLE_STATS so OptimizationRemarkMissed is skipped in LLVM_ENABLE_STATS==0 builds.
If these OptimizationRemarkMissed are useful and not noisy, we can replace llvm::Statistic with llvm::TrackingStatistic, or use a different abstraction to keep track of the strings.

Diff Detail

Event Timeline

MaskRay created this revision.Apr 23 2021, 5:05 PM
MaskRay requested review of this revision.Apr 23 2021, 5:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 23 2021, 5:05 PM
MaskRay edited the summary of this revision. (Show Details)Apr 23 2021, 5:14 PM
MaskRay edited the summary of this revision. (Show Details)Apr 23 2021, 5:29 PM
lattner accepted this revision.Apr 23 2021, 5:35 PM

This looks like a reasonable cleanup to me. I'd recommend #ifdef'ing out the using namespace declaration as well.

This revision is now accepted and ready to land.Apr 23 2021, 5:35 PM
dblaikie added inline comments.
llvm/lib/Transforms/Scalar/LoopFuse.cpp
1540–1546

Losing this remark seems unfortunate - looks like this contradicts the statement in the patch description that these pointers are unused? Mostly unused, but not entirely.

Perhaps this use could be migrated to something else? Or perhaps the Statistic classes could store the strings as char[] members (using some SFINAE/factory template to get the right sizes) so they aren't pointers - if that would address the issue?

MaskRay added inline comments.Apr 24 2021, 4:24 PM
llvm/lib/Transforms/Scalar/LoopFuse.cpp
1540–1546

LoopFuse is one of the disabled loop passes (https://lists.llvm.org/pipermail/llvm-dev/2021-April/149737.html).
We don't know how well it works. We don't know whether using OptimizationRemarkMissed will be noisy or not (feasible for optimization remarks). Replacing llvm::Statistic with llvm::TrackingStatistic can make such usage work, but I am not sure it is suitable. So I choose the simplest approach #if LLVM_ENABLE_STATS.

MaskRay added inline comments.
llvm/lib/Transforms/Scalar/LoopFuse.cpp
1540–1546
dblaikie added inline comments.Apr 24 2021, 4:39 PM
llvm/lib/Transforms/Scalar/LoopFuse.cpp
1540–1546

Fair enough - if it's untested and unused it's hardly the end of the world & noteworthy if this is the odd one out in terms of using the statistics name/etc even when stats aren't enabled.

I don't think this has to be addressed to ship this given all that - but might be worth clarifying/adding the nuance to the patch description/commit message.

MaskRay edited the summary of this revision. (Show Details)Apr 25 2021, 11:46 AM
MaskRay marked an inline comment as done.
MaskRay edited the summary of this revision. (Show Details)Apr 26 2021, 1:33 PM

"has 3 unused pointers" is the bit I had in mind in terms of modifying the description - that statement seems incorrect to me (in the presence of the LoopFuse usage). So I was thinking something like "has 3 mostly unused pointers" and then down at the end

"the one use of these pointers is in LoopFuse (an unused pass) for printing out an optimization remark - for now that usage has been #ifdef'd out, if it's desirable to have the remark, it could use a different abstraction to keep track of the strings, or these particular statistics could be hardcoded to TrackingStatistic"

This revision was automatically updated to reflect the committed changes.

"has 3 unused pointers" is the bit I had in mind in terms of modifying the description - that statement seems incorrect to me (in the presence of the LoopFuse usage). So I was thinking something like "has 3 mostly unused pointers" and then down at the end

"the one use of these pointers is in LoopFuse (an unused pass) for printing out an optimization remark - for now that usage has been #ifdef'd out, if it's desirable to have the remark, it could use a different abstraction to keep track of the strings, or these particular statistics could be hardcoded to TrackingStatistic"

Oh, did not see this when I pushed the commit (I had a script running the last round of testing before pushing).

Yes, has 3 mostly unused pointers would be more accurate

"the one use of these pointers is in LoopFuse (an unused pass) for printing out an optimization remark - for now that usage has been #ifdef'd out, if it's desirable to have the remark, it could use a different abstraction to keep track of the strings, or these particular statistics could be hardcoded to TrackingStatistic"

This description would be more accurate as well:)

MaskRay edited the summary of this revision. (Show Details)Apr 26 2021, 1:47 PM

@rriddle llvm+clang has ~1000 llvm::Statistic global variables. This patch can avoid the space consumption (24KiB in .bss, and more in .rodata) in a -DLLVM_ENABLE_ASSERTIONS=off build because the pointers are mostly unused.

mlir/lib/Pass/PassStatistics.cpp seems to dump the names even in -DLLVM_ENABLE_ASSERTIONS=off builds? Those statistics will have names but no counts. If the statistics are not useful in -DLLVM_ENABLE_ASSERTIONS=off builds, I'll just remove using llvm::Statistic::getDebugType; from mlir/include/mlir/Pass/Pass.h and reland this.

@rriddle llvm+clang has ~1000 llvm::Statistic global variables. This patch can avoid the space consumption (24KiB in .bss, and more in .rodata) in a -DLLVM_ENABLE_ASSERTIONS=off build because the pointers are mostly unused.

mlir/lib/Pass/PassStatistics.cpp seems to dump the names even in -DLLVM_ENABLE_ASSERTIONS=off builds? Those statistics will have names but no counts. If the statistics are not useful in -DLLVM_ENABLE_ASSERTIONS=off builds, I'll just remove using llvm::Statistic::getDebugType; from mlir/include/mlir/Pass/Pass.h and reland this.

Yeah, I would just remove that and re-land. Do whatever is easiest for you.