[PATCH] D92998: [CSSPGO][llvm-profgen] Pseudo probe based CS profile generation

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/test/tools/llvm-profgen/noinline-cs-pseudoprobe.test:13-14
+; CHECK-NEXT: 1: 15
+; CHECK-NEXT: 2: 0
+; CHECK-NEXT: 3: 0
+; CHECK-NEXT: 4: 15
----------------
wmi wrote:
> In [main:2 @ foo], probes with 0 count are not dumped. In which case probe with 0 count will be dumped?
The 0 count is for a dangling probe case to distinguish a missing count, see the ProfileGenerator.cpp:420

Copied the comments for you to refer:

```
    // Drop the samples collected for a dangling probe since it's misleading.
    // We still report the probe but with a special zero count. The compiler
    // won't trust the zero count and will rely on the counts inference
    // algorithm to get the probe a reasonable count. Note that a zero count is
    // different from a missing count, where the latter really tells the
    // compiler that a probe is never executed.
```


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:347-349
+      // PrefixContextId is the context id string except for the leaf probe's
+      // context, the final ContextId will be:
+      // ContextId =  PrefixContextId + LeafContextId;
----------------
wmi wrote:
> Move it to header comment of extractPrefixContextId in case extractPrefixContextId is used elsewhere.
Good catch, moved.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:139
+  FunctionSamples &
+  getFunctionProfileForLeaf(StringRef PrefixContextId,
+                            SmallVector<std::string, 16> &LeafInlinedContext,
----------------
wmi wrote:
> Make it an overload function of getFunctionProfileForLeafProbe so it is known to be used for probe?
Good suggestion!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92998