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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
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);
----------------
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.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:249
 
   // SeenMetadata tracks whether we have processed metadata for the current
   // top-level function profile.
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > update comment
> > Good catch, done.
> I'm still seeing SeenMetadata in the comment?
Good catch, changed to DepthMetadata.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1484
           Changed = true;
+        } else if (FunctionSamples::ProfileIsPreinlined) {
+          LocalNotInlinedCallSites.try_emplace(I, FS);
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > hoy wrote:
> > > > wenlei wrote:
> > > > > The definition for `ProfileIsPreinlined` isn't very clear. A CS profile can also be preinlined, but here it looks like you're using it specifically for flat profile that is preinlined. Suggest we adhere to the literal definition which would apply to both flat and nest profile for preinline.
> > > > > 
> > > > > The check here could be using whether context tracker is available, basically we're try to track context without context tracker (which is tied to flat CS profile).
> > > > Yeah, `ProfileIsNestedCS` sounds better?
> > > > 
> > > > Makes sense to check against `ContextTracker`.
> > > > Yeah, ProfileIsNestedCS sounds better?
> > > 
> > > That works. Though it doesn't make sense for `ProfileIsNestedCS` to be true while `ProfileIsCS` is false. So if we use `ProfileIsNestedCS`, we need to either 1) rename `ProfileIsCS` to be `ProfileIsFlatCS`, or 2) replace check on `ProfileIsCS` with `ProfileIsCS & !ProfileIsNestedCS` 
> > `ProfileIsFlatCS` and `ProfileIsNestedCS` sounds good to me.
> nit: there's NestedCS and CSFlat (as opposed to FlatCS) in switch, section flags and variable names , which is a bit inconsistent. 
Yeah, we still use the switch gen-nested-cs-profile. Let me change it to gen-cs-nested-profile


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115205