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


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




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


================
Comment at: llvm/test/tools/llvm-profdata/Inputs/cs-sample-preinline.proftext:16
  15: 11
- !Attributes: 1
+ !Attributes: 2
 [external:12 @ main]:154:12
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > what triggered these attribute value changes?
> > It's due to the following change in SampleProfWriter.cpp:
> > 
> > 
> > ```
> > if (S.getContext().getAllAttributes()) {
> >     OS.indent(Indent + 1);
> >     OS << "!Attributes: " << S.getContext().getAllAttributes() << "\n";
> > ```
> I meant the value change from 0/1 to 2, not the ones that were 0. 
Ah, this is the input profile of a test. I made it to be preinliner-generated. It was copied from cs-sample.proftext.


================
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"));
----------------
wenlei wrote:
> how about `gen-nested-cs-profile` to make it explicit and differentiate from non-CS profile, especially if we go with `ProfileIsNestedCS` global flag.
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115205