[PATCH] D146452: [AutoFDO] Use flattened profiles for profile staleness metrics

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1333
+
+  static void flattenNestedProfile(SampleProfileMap &OutputProfiles,
+                                   const FunctionSamples &FS) {
----------------
wenlei wrote:
> This function doesn't have check on whether input is CS or profile or not, and it relies on caller to pass in the right kind of profile. Should we make it private and only expose flattenProfile as API which can take any kind of profile as input.
Sounds good, moved to private scope.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1351
+      }
+    }
+
----------------
hoy wrote:
> nit: assert CallsiteSamples is empty?
Sounds good, assertion added


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1353-1356
+    // TotalSamples might not be equal to the sum of all samples from
+    // BodySamples and CallsiteSamples. So here we use "TotalSamples =
+    // Original_TotalSamples - All_of_Callsite_TotalSamples +
+    // All_of_Callsite_HeadSamples" to compute the new TotalSamples.
----------------
wenlei wrote:
> Is the total samples populated this way going to be consistent with total samples from profiles generated directly from llvm-profgen?
I think so.  Supposing TotalSamples = TotalBodySamples + TotalCallsiteSamples. Remembering we discussed this while implementing llvm-profgen, the TotalBodySamples usually is not the sum of all bodySamples,  we can't sum the bodySamples at the end for this, so here use the reversed way: use TotalSamples minus TotalCallsiteSamples to recover the totalBodySamples.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:1039
+    ProfileConverter::flattenProfile(ProfileMap, FunctionSamples::ProfileIsCS);
+    if (FunctionSamples::ProfileIsCS)
+      ProfileIsCS = FunctionSamples::ProfileIsCS = false;
----------------
wenlei wrote:
> this check seems unnecessary
removed.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:1254
       cl::desc("Generate nested function profiles for CSSPGO"));
+  cl::opt<bool> GenFlattenedProfile(
+      "gen-flattened-profile", cl::init(false),
----------------
hoy wrote:
> wenlei wrote:
> > do we need to make sure `gen-flattened-profile` and `gen-cs-nested-profile` won't/can't be used at the same time? 
> > 
> > also wondering if we should consolidate them into one flag, something like `profile-structure=<cs-nested|flat>`.
> > also wondering if we should consolidate them into one flag, something like profile-structure=<cs-nested|flat>.
> 
> This is a good point. There's a similar term `SampleProfileLayout` introduced https://reviews.llvm.org/D121651 . Consider unifying them like below?
> 
> 
> ```
> enum SampleProfileLayout {
>   SPL_None = 0,
>   SPL_Nest = 0x1,
>   SPL_Flat = 0x2,
>   SPL_CSFlat = 0x3,
> };
> ```
> 
Good idea! The implementation in https://reviews.llvm.org/D121651 looks good to me, so I copied  that (except the "CSFlat", will leave it to the original patch), but I have scratching my head with the term `nest`, `CS-nested`, `flat`, `cs-flat`...I will follow your decision. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146452