[PATCH] D100235: [CSSPGO][llvm-profgen] Always report dangling probes for frames with real samples.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:518
+    // Ignore dangling probes since they will be reported later if needed.
+    if (Probe->isDangling())
+      continue;
----------------
wenlei wrote:
> wlei wrote:
> > Try to understand this.
> > 
> > If pass2's probe is dangling, then return 0 no matter what in the profile(pass1);
> > 
> > If pass2's probe is not dangling and reading from profile gets a dangling probe(pass1), then return invalid count(UINT64_MAX), (this case happen because later the BB of the probe will be removed)
> > 
> > is that right? 
> > 
> > So this change make sure all the future removed BB will be identified as dangling so that the inference algorithm can work on this?
> > 
> > is this issue also coming from the rebalancing algorithm? 
> > 
> > like we have TrueBranch :0 and FalseBranch: 0, the rebalancing algorithm can't work on this.
> > 
> > but if we have TrueBranch:0 and FalseBranch:dangling, then it can be rebalanced.
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> By return, you meant the return value of `SampleProfileLoader::getProbeWeight`?
> 
> For inference, I think what matters is whether probe is dangling from pass1. 
Here is only about pass1. llvm-profgen runs on pass1 binary and perf data. A dangling probe here is from pass1. 

When the dangling-ness is imported in pass2, the compiler has to guess its sample count instead of trusting what's collected in pass1. When the same probe is seen also dangling in pass2 compilation, it indicates that the code being probed is removed by pass2, therefore, no matter what samples pass1 collected for it, it shouldn't matter since pass2 is no longer interested in the probe.

The issue was noticed when a dangling probe was not reported dangling, thus the counts inferencer trust its zero sample count instead of inferring a count for it. Though I didn't see it directly related to rebalancing, I believe it will fix your example case.


================
Comment at: llvm/tools/llvm-profgen/PseudoProbe.h:48
   };
-  std::unordered_map<InlineSite, std::unique_ptr<PseudoProbeInlineTree>,
-                     InlineSiteHash>
-      Children;
+  using ChildrenType =
+      std::unordered_map<InlineSite, std::unique_ptr<PseudoProbeInlineTree>,
----------------
wenlei wrote:
> nit: a more reflective type name, something like `InlinedProbesMap`, `InlinedProbeTreeMap`?
`InlinedProbeTreeMap` sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100235