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

[Wenlei He via Phabricator via llvm-commits]

wenlei 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());
----------------
> 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`? 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:92
+static cl::opt<bool> UpdateCallsiteSamples(
+    "update-callsite-samples", llvm::cl::init(false),
+    llvm::cl::desc(
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122609