[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:718
+  if (!FProfile) {
+    FProfile = new FunctionSamples();
+    FProfile->setName(ContextNode->getFuncName());
----------------
The FunctionSamples is `new` here.


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







================
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
----------------
wenlei wrote:
> 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?
Right, we use the callee's body sample to update caller's body sample.

It is a bug in previous implementation, but it should not be critical one, as you said, it's only related with the first body samples.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125246