[ADT] Remove StatisticBase and make NoopStatistic empty

[MaskRay]

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.

[lattner]

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

[dblaikie]

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]

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]

llvm/lib/Transforms/Scalar/LoopFuse.cpp

1540–1546

@kbarton

[dblaikie]

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.

[dblaikie]

"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"

[MaskRay]

In D101211#2717753, @dblaikie wrote:

"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:)

[antiagainst]

FYI: this breaks MLIR build. I'll revert it for now.

https://buildkite.com/mlir/mlir-core/builds/13299#ad0f8901-dfa4-43cf-81b8-7940e2c6c15b

[MaskRay]

@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]

In D101211#2718227, @MaskRay wrote:

@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.

llvm/include/llvm/ADT/Statistic.h

 #endif
 
 namespace llvm {
 
 class raw_ostream;
 class raw_fd_ostream;
 class StringRef;
 
-class StatisticBase {
+class TrackingStatistic {
 public:
-  const char *DebugType;
-  const char *Name;
-  const char *Desc;
+  const char *const DebugType;
+  const char *const Name;
+  const char *const Desc;
 
-  StatisticBase(const char *DebugType, const char *Name, const char *Desc)
-      : DebugType(DebugType), Name(Name), Desc(Desc) {}
-
-  const char *getDebugType() const { return DebugType; }
-  const char *getName() const { return Name; }
-  const char *getDesc() const { return Desc; }
-};
-
-class TrackingStatistic : public StatisticBase {
-public:
   std::atomic<unsigned> Value;
   std::atomic<bool> Initialized;
 
   TrackingStatistic(const char *DebugType, const char *Name, const char *Desc)
-      : StatisticBase(DebugType, Name, Desc), Value(0), Initialized(false) {}
+      : DebugType(DebugType), Name(Name), Desc(Desc), Value(0),
+        Initialized(false) {}
+
+  const char *getDebugType() const { return DebugType; }
+  const char *getName() const { return Name; }
+  const char *getDesc() const { return Desc; }
 
   unsigned getValue() const { return Value.load(std::memory_order_relaxed); }
 
   // Allow use of this class as the value itself.
   operator unsigned() const { return getValue(); }
 
   const TrackingStatistic &operator=(unsigned Val) {
     Value.store(Val, std::memory_order_relaxed);
TrackingStatistic &init() {
     if (!Initialized.load(std::memory_order_acquire))
       RegisterStatistic();
     return *this;
   }
 
   void RegisterStatistic();
 };
 
-class NoopStatistic : public StatisticBase {
+class NoopStatistic {
 public:
-  using StatisticBase::StatisticBase;
+  NoopStatistic(const char * /*DebugType*/, const char * /*Name*/,
+                const char * /*Desc*/) {}
 
   unsigned getValue() const { return 0; }
 
   // Allow use of this class as the value itself.
   operator unsigned() const { return 0; }
 
   const NoopStatistic &operator=(unsigned Val) { return *this; }
 

llvm/lib/Transforms/Scalar/LoopFuse.cpp

void invalidate() {
     MemWrites.clear();
     MemReads.clear();
     Valid = false;
   }
 
   bool reportInvalidCandidate(llvm::Statistic &Stat) const {
     using namespace ore;
     assert(L && Preheader && "Fusion candidate not initialized properly!");
+#if LLVM_ENABLE_STATS
     ++Stat;
     ORE.emit(OptimizationRemarkAnalysis(DEBUG_TYPE, Stat.getName(),
                                         L->getStartLoc(), Preheader)
              << "[" << Preheader->getParent()->getName() << "]: "
              << "Loop is not a candidate for fusion: " << Stat.getDesc());
+#endif
     return false;
   }
 };
 
 struct FusionCandidateCompare {
   /// Comparison functor to sort two Control Flow Equivalent fusion candidates
   /// into dominance order.
   /// If LHS dominates RHS and RHS post-dominates LHS, return true;
#endif
   ///    <path/filename>:<line number>:<column number>: [<function name>]:
   ///       <Cand1 Preheader> and <Cand2 Preheader>: <Stat Description>
   template <typename RemarkKind>
   void reportLoopFusion(const FusionCandidate &FC0, const FusionCandidate &FC1,
                         llvm::Statistic &Stat) {
     assert(FC0.Preheader && FC1.Preheader &&
            "Expecting valid fusion candidates");
     using namespace ore;
+#if LLVM_ENABLE_STATS
     ++Stat;
     ORE.emit(RemarkKind(DEBUG_TYPE, Stat.getName(), FC0.L->getStartLoc(),
                         FC0.Preheader)
              << "[" << FC0.Preheader->getParent()->getName()
              << "]: " << NV("Cand1", StringRef(FC0.Preheader->getName()))
              << " and " << NV("Cand2", StringRef(FC1.Preheader->getName()))
              << ": " << Stat.getDesc());
+#endif
   }
 
   /// Fuse two guarded fusion candidates, creating a new fused loop.
   ///
   /// Fusing guarded loops is handled much the same way as fusing non-guarded
   /// loops. The rewiring of the CFG is slightly different though, because of
   /// the presence of the guards around the loops and the exit blocks after the
   /// loop body. As such, the new loop is rewired as follows: