[PATCH] D122609: [llvm-profgen] An option to update callsite body samples by summing up all call target samples.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:751
+      uint64_t TargetSamples = I.second.getCallTargetSamples();
+      if (TargetSamples > I.second.getSamples())
+        I.second.addSamples(TargetSamples - I.second.getSamples());
----------------
wenlei wrote:
> > LBR ranges is formed from two consecutive branch samples. Therefore the last entry in a LBR record will not be counted towards body samples while there's still a chance for it to be counted towards call targets if it is a function call 
> 
> Based on this theory, is it actually possible for `I.second.getSamples()` to be larger than `TargetSamples`? 
Actually it is possible. If some call targets are external targets, they won't be counted, but the body sample count which is from the lbr ranges can include them. 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:92
+static cl::opt<bool> UpdateCallsiteSamples(
+    "update-callsite-samples", llvm::cl::init(false),
+    llvm::cl::desc(
----------------
wenlei wrote:
> I'm not sure if we need a switch here. It feels to me that we should always try to make the counts consistent. But ofc, that means more testing is needed.
Makes sense to make it unconditional. 

Regarding testing, it should have minimal impact on CS probe profile. I'll test for regular non-CS profile.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:97
+
+  void updateSamples();
+
----------------
wenlei wrote:
> nit: updateFunctionSamples?
done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122609