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

[Wenlei He via Phabricator via llvm-commits]

wenlei 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);
----------------
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?  


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:537-543
+    // Contexts coming with a `ContextShouldBeInlined` attribute indicate this
+    // is a preinliner-computed profile.
+    if (OrigChildContext.hasAttribute(ContextShouldBeInlined))
+      FunctionSamples::ProfileIsPreinlined = true;
+
+    // Remove the original child profile.
+    ProfileMap.erase(OrigChildContext);
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > IIUC, the per-context ContextShouldBeInlined now becomes a per-profile flag. However, shouldInlineCandidate used by the inliner still checks attribute ContextShouldBeInlined from sample's context. That was how we tell inliner to honor preinline decision. Now without that attribute, how does inliner know to honor preinline decision?
> > > The per-profile flag is only used to tell the compiler how to set up the inliner (priority vs non-prioirty) and mcf extra. When it comes to inlining a particular callsite, the inliner would not honor the inline decision based on the per-context attribute. We always encode the per-context attribute in the profile.
> > > When it comes to inlining a particular callsite, the inliner would not honor the inline decision based on the per-context attribute. 
> > 
> > Did you mean would or would not? I expect the correct behavior to be that inliner will honor per-context `ContextShouldBeInlined` attribute, which is what happens with flat CS profile.
> > 
> > > We always encode the per-context attribute in the profile. 
> > 
> > Where is the per-context attribute transferred from flat profile to nest profile? This is the only place I see we check `ContextShouldBeInlined`, but it's transferred to global `FunctionSamples::ProfileIsPreinlined`, not per-context attribute. 
> > 
> > Did you mean would or would not? I expect the correct behavior to be that inliner will honor per-context ContextShouldBeInlined attribute, which is what happens with flat CS profile.
> 
> I mean would. Sorry for the confusion.
> 
> 
> > Where is the per-context attribute transferred from flat profile to nest profile? This is the only place I see we check ContextShouldBeInlined, but it's transferred to global FunctionSamples::ProfileIsPreinlined, not per-context attribute.
> 
> The attribute that comes with a context in a flat profile will be encoded with the context into the nested profile as well. Note that we only reset the name of the original context but nothing else. The original attribute is still there.
> 
> 
> ```
> // Reset the child context to be contextless.
>     ChildProfile->getContext().setName(OrigChildContext.getName());
> ```
> 
> See the test llvm/test/tools/llvm-profdata/cs-sample-nested-profile.test . 
> 
> 
Now I see how the attributed is transferred, thanks for clarification. 


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


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1484
           Changed = true;
+        } else if (FunctionSamples::ProfileIsPreinlined) {
+          LocalNotInlinedCallSites.try_emplace(I, FS);
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115205