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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:947
+    NewProfile.setContext(FContext);
+    delete (FProfile);
+    Node.setFunctionSamples(nullptr);
----------------
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?


================
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
----------------
wlei wrote:
> hoy wrote:
> > PLease add more comments about why the processing order matters.
> Comments updated.
> parent(inliner)'s sample depends on child(inlinee)'s sample, so traverse the tree in post-order.

I thought order was not important because we use head samples to update body samples. But it looks like we use entry samples which could be first body samples. 

So this was a bug in previous implementation?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125246