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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1183
 
+class CSProfileConverter {
+public:
----------------
High level comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:1187
+  void convertProfiles();
+  struct FrameNode {
+    FrameNode(StringRef FName = StringRef(),
----------------
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..)


================
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, "
----------------
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."


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


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


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


================
Comment at: llvm/lib/ProfileData/SampleProfReader.cpp:346
           FProfile.setFunctionHash(FunctionHash);
-          ++ProbeProfileCount;
+          if (Depth == 1)
+            ++ProbeProfileCount;
----------------
Does this mean we only set function hash for top level profile? why skip setting hash for nested inlinees?


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


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


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


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


================
Comment at: llvm/test/tools/llvm-profdata/Inputs/cs-sample-preinline.proftext:16
  15: 11
- !Attributes: 1
+ !Attributes: 2
 [external:12 @ main]:154:12
----------------
what triggered these attribute value changes?


================
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
----------------
have a probe version test case too to cover read/write per-inlinee function hash? 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:757
+    FunctionSamples::ProfileIsCS = false;
+    FunctionSamples::ProfileIsPreinlined = EnableCSPreInliner;
+  }
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115205