[PATCH] D94435: [SampleFDO] Add the support to split the function profiles with context into separate sections.

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:2126
+  if (!F.getEntryCount().hasValue())
+    F.setEntryCount(ProfileCount(initialEntryCount, Function::PCT_Real));
   std::unique_ptr<OptimizationRemarkEmitter> OwnedORE;
----------------
hoy wrote:
> wmi wrote:
> > hoy wrote:
> > > `F` may get a chance to update its entry count with a post-inline count during top-down processing with the default layout. Maybe put this under the check against `SkipNoContextProf`?
> > I think for any case we don't want to reinitiate F's entrycount to initialEntryCount if it has already had a valid value.
> > 
> > Even if SkipNoContextProf is false, in LTO/ThinLTO postlink phase, it is possible that the emitAnnotations function doesn't change anything which could affect profile annotation (i.e., the variable Changed in emitAnnotations is false), so if a function getting a valid entry count in prelink phase is reinitialized to -1 before entering emitAnnotations in postlink,  emitAnnotations may not be able to update it with a valid entry count again.
> > 
> > In addition, if F has a valid entrycount entering emitAnnotations, emitAnnotations can still update it without problem.
> Oh yeah, entrycount can be set in `emitAnnotations`. Thanks for pointing it out.
> 
> I was wondering if `F`'s profile can change due to the counts returning from its inliner in postlink phase, thus it may need an update in postlink though itself doesn't have context samples. That might happen with fullLTO but with thinLTO, the counts returning is already less accurate since it cannot be done cross-threads. 
> I was wondering if F's profile can change due to the counts returning from its inliner in postlink phase, thus it may need an update in postlink though itself doesn't have context samples. 

I think In this case, `emitAnnotations` should still be the right place where the update happens, though we don't capture such profile merge as "change" today and `emitAnnotations` won't be triggered. 


================
Comment at: llvm/test/Transforms/SampleProfile/ctxsplit.ll:3
+; postlink phase while flattened part of the ctxsplit profile will not be read.
+; RUN: opt < %s -passes='thinlto<O2>' -pgo-kind=pgo-sample-use-pipeline -profile-file=%S/Inputs/ctxsplit.extbinary.afdo -S | FileCheck %s --check-prefix=POSTLINK
+;
----------------
I noticed the input is a binary profile. Is the new layout of extended binary profile still two-way convertible with text profile? Text profile works better for small test cases? 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D94435