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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:397
 
+      SampleContext &ContextOnNode =
+          FromNode->getFunctionSamples()->getContext();
----------------
wlei wrote:
> hoy wrote:
> > wlei wrote:
> > > 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.
> > > 
> > > 
> > > 
> > > 
> > > SA's context is marked MergedContext and Now SA's node is pointed to NB(NA is removed)
> > 
> > Why SA's should point to NB? Can NA be kept so that SA always point to it?
> > 
> > When a context is merged to another context, the context should never be used anywhere. What's the benefit of making it point to the other tri node?
> > 
> > 
> > 
> We need to update FSamples to always point to the new ContextTrieNode, that's why we use `updateContextNode`.  This is because we iterate the `FuncToCtxtProfiles` which is a list of FSamples to be merged to get base sample.  Since the the node can be merged dynamically, take the previous example. the NB after merging is the sum of (SA + SB), NA is the old SA, it should be removed. but when to get FA's Node, if we get the old SA, it will be merged to NA again becoming (2*SA +SB). so we need to always make point to the node on the trie.  And now both SA and SB are pointing to NB, then here use the oldcontext(`MergedContext `)  to filter out one.
> but when to get FA's Node, if we get the old SA, it will be merged to NA again becoming (2*SA +SB).

At this time. SA's context should be marked "MergedContext". Can we skip it based on the flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127031