[PATCH] D101211: [ADT] Remove StatisticBase and make NoopStatistic empty

Sat Apr 24 16:39:42 PDT 2021

dblaikie added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1540-1545
     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());
----------------
MaskRay wrote:
> MaskRay wrote:
> > dblaikie wrote:
> > > 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?
> > 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`.
> @kbarton 
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101211/new/

https://reviews.llvm.org/D101211