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

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:389
+      // Check to avoid re-merge any context.
+      if (OldContext.hasState(MergedContext))
         continue;
----------------
hoy wrote:
> Should we also check InlinedContext here?
Answering in the comment below.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:397
 
+      SampleContext &ContextOnNode =
+          FromNode->getFunctionSamples()->getContext();
----------------
hoy wrote:
> I'm a bit confused here. Is ContextOnNode same with OldContext? Should a FunctionSample, its SampleContext and ContextTrieNode stick together as a whole? It may be worth a comment here. 
> 
Supposing Node(NA) owned a FunctionSamples(SA) and NB owns SB, NA is merged into NB.

SA's context is marked `MergedContext` and Now SA's node is pointed to NB(NA is removed)

so here `OldContext` is the context of NA,  `ContextOnNode` is the context of NB(might be marked `InlinedContext` in the future). 

will comment using this example.

> Should a FunctionSample, its SampleContext and ContextTrieNode stick together as a whole
It seems they can't be sticked together if we want to keep the old status(`MergedContext`) and the node pointer always point to the new on trie node.






Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127031