[PATCH] D115205: [CSSPGO] Use nested context-sensitive profile.

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.

lgtm, thanks.



================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:534
+    // profile in the prelink phase for to-be-fully-inlined functions.
+    if (!NodeProfile || DuplicateContextProfilesIntoBaseProfile)
+      ProfileMap[ChildProfile->getContext()].merge(*ChildProfile);
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > hoy wrote:
> > > > > wenlei wrote:
> > > > > > When NodeProfile (parent profile) doesn't exist, would it be beneficial to create the NodeProfile to contain inlinee's profile? That would be a more faithful conversion from a flat profile. 
> > > > > When NodeProfile doesn't exist, it means the parent function is cold, and its profile also doesn't exist in the flat profile. So we are honoring that by not creating such profile here.
> > > > If we have preinliner on, cold parent should be removed from context already. If the parent exists, that means it's somewhat relevant for inlining.
> > > Right. Here we are not necessarily converting a preinlined flat profile. When the flat profile is preinlined, we'll set the ProfileIsNestedCS flag, otherwise nodes in the context trie could have no profiles.
> > So at least in preinliner case, it could be better to keep parent profile?  
> Yes, that's currently it is. With preinliner, every node in the context trie will come with a profile so `NodeProfile` will never be empty. Without preinliner, `NodeProfile` could be empty.
ok, sounds good. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115205