[PATCH] D119494: [CSSPGO] Do not recount callee samples when computing profile summary for nested CS profile.

Thu Feb 10 18:47:27 PST 2022

hoy added inline comments.

================
Comment at: llvm/lib/ProfileData/ProfileSummaryBuilder.cpp:124
+  // profiles. This can happen during the generation of CS nested profile.
+  if (!GenCSNestedProfile || !GenerateMergedBaseProfiles)
+    for (const auto &I : FS.getCallsiteSamples())
----------------
hoy wrote:
> wenlei wrote:
> > We should not have ProfileSummaryBuilder depending on profile generation flags from llvm-profgen. You moved the flags out off llvm-profgen to avoid cyclic dependency, but this is still hacky. 
> > 
> > If we want to this specific behavior of ProfileSummaryBuilder, i.e. whether callee samples should be counted, we can have ProfileSummaryBuilder expose a proper knob for that. 
> > 
> > In this case, maybe we could also consider actually setting a duplicated bit/attribute for those duplicated callee profile, then ProfileSummaryBuilder can naturally skip the duplicated one, which should be equivalent. 
> For extbiny profile, ProfileSummaryBuilder is called in profile writing time, which is still in the SampleProf library. The command line switches being checked here is because we would like to correct the summary computation during profile writing time. Maybe we can expose a writer flag to for this instead.
> 
> A function profile may have both inlined samples and uninlined samples and it's hard to tell them apart once they are merged. We could add an extra attribute to the extbin profile for GenerateMergedBaseProfiles which can be checked here, similar to `ProfileIsCSNested`. This seems cleaner. WDYT?
Actually an attribute for each callee profile may work better. It can help text profile too.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119494