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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1187
+  void convertProfiles();
+  struct FrameNode {
+    FrameNode(StringRef FName = StringRef(),
----------------
wenlei wrote:
> FunctionSamples already has tree structure, can we use FunctionSamples tree to hold the nesting profiles directly, instead of introducing an intermediate trie tree? (The intermediate trie tree also overlaps with ContextTrieNode..)
Nodes in a context tree may not have a function profile. Right, the struct here basically does what `ContextTrieNode` do. I was thinking about making `ContextTrieNode` shared between ProfilingTools and IPO, and that would require moving the definition into SampleProf.h, which may not be what we want.


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:39
+cl::opt<bool> DuplicateContextProfilesIntoBaseProfile(
+    "duplicate-contexts-into-base", cl::init(true), cl::ZeroOrMore,
+    cl::desc("When converting a CS flat profile into a nested profile, "
----------------
wenlei wrote:
> nit: `generate-merged-base-profiles` - "When generating nested context-sensitive profiles, always generate extra base profile for function with all its context profiles merged into it."
Sounds good.


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


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


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


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:346
           FProfile.setFunctionHash(FunctionHash);
-          ++ProbeProfileCount;
+          if (Depth == 1)
+            ++ProbeProfileCount;
----------------
wenlei wrote:
> Does this mean we only set function hash for top level profile? why skip setting hash for nested inlinees?
We set function hash for all profiles. We just count top-level profiles by `ProbeProfileCount` for sanity check in the assert several lines below. I just renamed the variable to `TopLevelProbeProfileCount`.


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:1091
+                                                   FunctionSamples *FProfile,
+                                                   bool Inlined) {
+  if (Data < End) {
----------------
wenlei wrote:
> This parameter is not used?
Good catch.


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


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1523
 
+void SampleProfileLoader::accumulateNonInlinedSamples(
+    DenseMap<CallBase *, const FunctionSamples *> NonInlinedCallSites,
----------------
wenlei wrote:
> nit: `promoteMergeNotInlinedContextSamples`. this is effectively a  "context tracker" for nest profile, so we can use similar name as context tracker. 
Sounds good.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1775
   DenseSet<GlobalValue::GUID> InlinedGUIDs;
-  if (ProfileIsCS && CallsitePrioritizedInline)
+  if (CallsitePrioritizedInline)
     Changed |= inlineHotFunctionsWithPriority(F, InlinedGUIDs);
----------------
wenlei wrote:
> This also opens up priority inliner for non-CS (not even preinlined) profile. Some (manual) testing as sanity check would be good. 
Test added to llvm-project/llvm/test/Transforms/SampleProfile/inline-mergeprof.ll and inline.ll


================
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:
> 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";
```


================
Comment at: llvm/test/tools/llvm-profdata/cs-sample-nested-profile.test:1
+RUN: llvm-profdata merge --sample --text -output=%t.proftext %S/Inputs/cs-sample-preinline.proftext --gen-nested-profile=1 -duplicate-contexts-into-base=0
+RUN: FileCheck %s < %t.proftext --match-full-lines --strict-whitespace
----------------
wenlei wrote:
> have a probe version test case too to cover read/write per-inlinee function hash? 
test added.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:757
+    FunctionSamples::ProfileIsCS = false;
+    FunctionSamples::ProfileIsPreinlined = EnableCSPreInliner;
+  }
----------------
wenlei wrote:
> The name ProfileIsPreinlined suggest it should be set for flat profile too when preinliner is on, but this seems to be only used for nested profile. 
The flag is supposed to be only used by the compiler to differentiate a nested CS profile from a nested non-CS profile. Maybe `ProfileIsNestedCS` sound better?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115205