[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:
> > 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.


================
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:
> > 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. 



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


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:952
+  cl::opt<bool> GenNestedProfile(
+      "gen-nested-profile", cl::Hidden, cl::init(false),
+      cl::desc("Generate nested function profiles for CSSPGO"));
----------------
how about `gen-nested-cs-profile` to make it explicit and differentiate from non-CS profile, especially if we go with `ProfileIsNestedCS` global flag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115205