[PATCH] D125246: [CSSPGO][llvm-profgen] Reimplement CS profile generator using context trie

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:881
+void CSProfileGenerator::inferFunctionSamplesOnTrie(ContextTrieNode *Node) {
+  // Child node should be processed before its parent, so traverse the tree in
+  // post-order
----------------
PLease add more comments about why the processing order matters.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:904
+  // in which case CallerProfile will not exist. But we can't modify
+  // ProfileMap while iterating it.
+  // TODO: created function profile for those callers too
----------------
Update the comment "But we can't modify ProfileMap while iterating it."?

On the other hand, since we are doing bottom-up processing, we should have no such limitation now?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:929
 
-    // If we already have head sample counts, we must have value profile
-    // for call sites added already. Skip to avoid double counting.
-    if (CalleeProfile.getHeadSamples())
-      continue;
-    // If we don't have context, nothing to do for caller's call site.
-    // This could happen for entry point function.
-    if (CalleeContext.isBaseContext())
-      continue;
+void CSProfileGenerator::buildProfileMapFromContextTrie(
+    ContextTrieNode *Node, SampleContextFrameVector &Context) {
----------------
nit: perhaps just name this function buildProfileMap since we are using trie by default.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:930
+void CSProfileGenerator::buildProfileMapFromContextTrie(
+    ContextTrieNode *Node, SampleContextFrameVector &Context) {
+  FunctionSamples *FProfile = Node->getFunctionSamples();
----------------
use reference type instead of pointer type for `Node` to explicitly indicate that this is a non-null ptr? 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:941
+    NewProfile.setContext(FContext);
+    delete (FProfile);
+  }
----------------
Can `FProfile` be updated in place to avoid copy and deletion?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:954
+  SampleContextFrameVector Context;
+  buildProfileMapFromContextTrie(&RootContext, Context);
+
----------------
I guess eventually this call will be after the preiliner run, but can this be moved after `computeSummaryAndThreshold` now? 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:954
+  SampleContextFrameVector Context;
+  buildProfileMapFromContextTrie(&RootContext, Context);
+
----------------
hoy wrote:
> I guess eventually this call will be after the preiliner run, but can this be moved after `computeSummaryAndThreshold` now? 
nit: seems that `Context` is not used anywhere. Perhaps make a wrapper function `void buildProfileMapFromContextTrie(    ContextTrieNode *Node)` and declare `Context` inside? 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:297
+  ContextTrieNode *
+  getContextNodeForContext(const SampleContextFrameVector &Context,
+                           bool WasLeafInlined = false);
----------------
nit: getOrCreateContextNodeForContext


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125246