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

[Hongtao Yu via Phabricator via llvm-commits]

hoy 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;
----------------
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. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D94435