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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:701
+  // by the path from root to this node.
+  ContextTrieNode *ContextNode;
   // State of the associated sample profile
----------------
wlei wrote:
> hoy wrote:
> > The field is only useful during context manipulation. Is it possible to store the link in an auxiliary map whereever it is used?
> I think we use context for two things 1)context manipulation(inlining) 2) reader and writer based on ProfileMap.
> My thought is `ContextNode` is the same thing to `FullContext`, i,e. we can always start from this node to iterate trie reversely to get the array of frames. I was wondering if we can use `ContextNode` to replace the `FullContext` in the future. And for sample reader and writer things, the key of the map can work as the FullContext, we might not need this field either. That's why I'm thinking to keep it here. WDYT?
yeah, I had the same thought that the two fields are kinda redundant to each other. I'm a bit worried about the compile-time memory increase. Would it be possible to use a union for the two fields for now?

> 2) reader and writer based on ProfileMap.

What is this usage?


> I was wondering if we can use ContextNode to replace the FullContext in the future.

It depends. For example, the profile merger probably does not want to always build a tri and merge multiple tries. The profile similarity checker may neither.



================
Comment at: llvm/lib/Transforms/IPO/SampleContextTracker.cpp:93
       FSamples->getContext().setState(SyntheticContext);
-      LLVM_DEBUG(dbgs() << "  Context promoted to: "
-                        << FSamples->getContext().toString() << "\n");
----------------
wlei wrote:
> hoy wrote:
> > may still be helpful to keep the debug dump?
> `moveToChildContext` is the function of `ContextTrieNode`, the `getContextString`  is in `SampleContextTracker`. Then it requires to pass `SampleContextTracker` to the `moveToChildContext` function. 
> 
> I just realize before it dump the promotion result for each node, but now we don't have the concept of promotion. Maybe dumping the the head node(ToNode) is enough?
> 
> 
Yeah, I don't have a strong opinion, as long as it makes your debug experience better :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127031