[PATCH] D127031: [CSSPGO][llvm-profgen] Reimplement SampleContextTracker using context trie

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:1008
+    } else {
+      ContextTracker.freeFSamples();
+    }
----------------
wlei wrote:
> hoy wrote:
> > I'm wondering if these two deallocations can be combined. Maybe in `buildProfileMap` we could exclude merged contexts there and then we don't need to do `Node->setFunctionSamples(nullptr);` inside `CSPreInliner::run`?
> The `merged contexts` node is removed from the trie after merging, so we can't traverse the whole trie to delete all the FSamples. That's why I was thinking to leverage the `FuncToCtxtProfiles` which stores all the FSample pointers at the beginning. And if it's not the preinliner pass, `FuncToCtxtProfiles` is empty, then we need to traverse the trie to delete them. 
> The merged contexts node is removed from the trie after merging, so we can't traverse the whole trie to delete all the FSamples.

I see. I guess we have to keep the diversion for now. A shared_ptr or a centralized FunctionSample storage such as what the reader maintains would typically help. That would require a change in `CSProfileGenerator::getOrCreateFunctionSamples` . Please leave a todo somewhere to be addressed laster.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127031