[PATCH] D138893: [llvm-profdata] Use flattening sample profile in profile supplementation

[Rong Xu via Phabricator via llvm-commits]

xur added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:929
 
   /// Return the maximum of sample counts in a function body including functions
   /// inlined in it.
----------------
davidxl wrote:
> The comment needs to be updated to reflect the new behavior.
Ack.


================
Comment at: llvm/test/tools/llvm-profdata/suppl-instr-with-sample-flatten.test:4
+
+Test all of goo's counters will be set to PseudoHot.
+RUN: llvm-profdata merge \
----------------
davidxl wrote:
> The test only sets bar.cc::bar's counter, not goo's?
Yes. That's a typo. I meant "bar.cc:bar".  Function "goo" is considered preinlined in FDO -- as it's not appear in FDO profile. We should not supplement goo.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:637
+  // profile does not have inlined callsite profiles.
+  // One caveat is the pre-inlined function -- their samples
+  // should be collapsed into the caller function.
----------------
davidxl wrote:
> pre-inlined, do you mean early-inlined?
I think we call "early-inline" pre-inliner in llvm. 


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:641
+  // info: the sum of entrycount and the max of maxcount.
+  // Here is the algorithm:
+  //   recursive (FS, root_name) {
----------------
davidxl wrote:
> Perhaps describe with a short example.
I will use the test case as the example. To be updated.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:663
+                                              const StringRef &RootName) {
+    auto BuildMaxSampleMapImpl = [&](const FunctionSamples &FS,
+                                     const StringRef &RootName,
----------------
davidxl wrote:
> There is no need for the nested lambda.
I think I need to use two level lambda if we want to use lamda. I can do with a member function but I need to pass many things around.


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

https://reviews.llvm.org/D138893