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

[Lei Wang via Phabricator via llvm-commits]

wlei 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
----------------
hoy wrote:
> PLease add more comments about why the processing order matters.
Comments updated.


================
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
----------------
hoy wrote:
> 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?
Good point! Now we can just create the new function samples for caller, actually that's already there see the later code creating this.

`FunctionSamples &CallerProfile = *getOrCreateFunctionSamples(CallerNode);`


================
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) {
----------------
hoy wrote:
> nit: perhaps just name this function buildProfileMap since we are using trie by default.
Sounds good!


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


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:941
+    NewProfile.setContext(FContext);
+    delete (FProfile);
+  }
----------------
hoy wrote:
> Can `FProfile` be updated in place to avoid copy and deletion?
the profile in ProfileMap require the full context as the key, so we can't create the value in `ProfileMap` in the early time. Or perhaps we can use the move semantics, but `FunctionSamples` currently doesn't have the move constructor, which will make things complicated. I think after the trimming and pre-inliner, the num of profile will be small, copy and deletion should be fine then?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:954
+  SampleContextFrameVector Context;
+  buildProfileMapFromContextTrie(&RootContext, Context);
+
----------------
hoy wrote:
> 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? 
Sounds good!


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:954
+  SampleContextFrameVector Context;
+  buildProfileMapFromContextTrie(&RootContext, Context);
+
----------------
wlei wrote:
> hoy wrote:
> > 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? 
> Sounds good!
> I guess eventually this call will be after the preiliner run, but can this be moved after computeSummaryAndThreshold now?

Yeah, we need to move it after preinliner and moreover it should be after `SampleContextTrimmer`.  

 `computeSummaryAndThreshold` is still using `ProfileMap` and I also found that `ProfileMap` can also be imported from the SampleReader. Both of them will needs to be rewritten. Maybe leave them a separate diff, just to be easy for the code review, what do you think?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125246