[PATCH] D138893: [llvm-profdata] Use flattening sample profile in profile supplementation
Rong Xu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 28 22:10:02 PST 2022
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
More information about the llvm-commits
mailing list