[PATCH] D130281: [NFC] FunctionSamples::getEntrySamples -> getEntryBBSampleCountEstimate

Thu Jul 21 11:24:57 PDT 2022

mtrofin added a comment.

In D130281#3669191 <https://reviews.llvm.org/D130281#3669191>, @wenlei wrote:

> Maybe getEntrySampleEstimate? the heuristic doesn't guarantee we get the counts for entry BB.

It does try to offer an estimate corresponding to that, though, if that's missing - hmmm... how about getHeadSamplesEstimate? It corresponds to what we call "head samples" in FunctionSamples (we're really not using "entry" in these APIs); it's discoverable right next to the getHeadSamples() API, and the "estimate" suffix should offer sufficient of a hint that it's a heuristic, and one would then be incentivized to read the docstring. I'd also, then, highlight in the comment the distinction between the two (i.e. one is raw data, while the "estimate" is, well, trying to produce a number using heuristic, etc)

WDYT?

================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:888
+  /// samples (i.e. getHeadSamples()), if non-zero.
+  uint64_t getEntryBBSampleCountEstimate() const {
     if (FunctionSamples::ProfileIsCS && getHeadSamples()) {
----------------
hoy wrote:
> nit: how about just name this `getEntrySamplesEstimate` ? `BB` is somehow implicated in the contexts where the API is used.
Right, but my goal is improving readability by making the name hint sufficiently well to what the function is about, without requiring the engineer wishing to use it also have to go see how it's used (and understand that code, etc).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130281