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

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

Thanks for the change -- good work. I can see how it improves performance of llvm-profgen. A few minor comments.



================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:718
+  if (!FProfile) {
+    FSamplesList.emplace_back(FunctionSamples());
+    FProfile = &FSamplesList.back();
----------------
`FSamplesList.emplace_back(FunctionSamples());`
->
`FSamplesList.emplace_back();`

`emplace_back` should be able to take ctor parameters directly, which avoids a copy/move. 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:932
 void CSProfileGenerator::populateInferredFunctionSamples() {
-  for (const auto &Item : ProfileMap) {
-    const auto &CalleeContext = Item.first;
-    const FunctionSamples &CalleeProfile = Item.second;
+  inferFunctionSamplesOnTrie(getRootContext());
+}
----------------
nit: we can simplify this by having a single `populateInferredFunctionSamples` function with default argument point to root context node. 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:947
+    NewProfile.setContext(FContext);
+    delete (FProfile);
+    Node.setFunctionSamples(nullptr);
----------------
wlei wrote:
> wenlei wrote:
> > Maybe it isn't too complicated to add move ctor for FunctionSamples? We're taking care of SampleContext here, and the rest are two std::map. But we can deal with it in separate change. 
> > 
> > Relatedly, this feels more like converting trie to map, rather than building a map from trie, because the process also "destroy" the trie. Who owns FProfile of the trie? While delete it on the fly saves a bit of mem, it also feels a bit weird for it to be deleted here as iiuc the trie may not own the underlying function samples?
> > Maybe it isn't too complicated to add move ctor for FunctionSamples? We're taking care of SampleContext here, and the rest are two std::map. But we can deal with it in separate change.
> OK, I just realized that we can use the default implicit move ctor of FunctionSamples, it can automatically move the two std::map. Then I think it should work here to add the std::move(..) to avoid copy. 
> 
> > Who owns FProfile of the trie?
> It's a little bit tricky. 
> 1) It can be from the ProfileMap(using --llvm-sample-profile as input) or sample reader from compiler side. we can't delete the profile owned by ProfileMap.
> 
> 2) for profile generation, it's created on on-the-fly on the trie.(see ProfileGenerator.cpp:718). That's why I delete this profile while "destroying" trie. 
> 
> I guess you meant the memory saving of the on-the-fly deletion is not noticeable(I agree that the memory after pre-inliner should be small), if so, I will separate this into two parts: 1) build map and 2) free the memory(in the future patch).
> 
> 
> 
> 
> 
Thanks for changing it to use move. With that the original trie still gets destroyed because it no longer holds valid FunctionSamples. So perhaps we should name this function "convert" instead of "build"? 

And since this is stateful, add an assertion/check to verify? like assert that ProfileMap is empty before we start the conversion, and the trie is still valid (holds valid function samples). 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.h:297
+  ContextTrieNode *
+  getContextNodeForContext(const SampleContextFrameVector &Context,
+                           bool WasLeafInlined = false);
----------------
wlei wrote:
> hoy wrote:
> > nit: getOrCreateContextNodeForContext
> Fixed!
nit: might want to unify the naming convention, `getOrCreateFunctionSamples` doesn't have `ForXYZ`. So `getOrCreateContextNode`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125246