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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

Thanks for addressing all the comments! Looks good to me now except for a couple minor things.



================
Comment at: llvm/include/llvm/Transforms/IPO/SampleContextTracker.h:95
   // deterministically.
-  using ContextSamplesTy = std::set<FunctionSamples *, ProfileComparer>;
+  using ContextSamplesTy = SmallVector<FunctionSamples *, 16>;
 
----------------
Wondering if 16 can be larger than needed in general. Since we are aiming for mem savings, maybe a smaller value is better? Or just use std::vector. Shouldn't be a big deal though.


================
Comment at: llvm/include/llvm/Transforms/IPO/SampleContextTracker.h:150
+  // Get a context string from root to current node.
+  std::string getContextString(const FunctionSamples &FSamples) const;
+  std::string getContextString(ContextTrieNode *Node) const;
----------------
Make this debug-only?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127031