[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:
> > 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.
> > 
> > Would it be possible to use a union for the two fields for now?
> Yeah, that's good to save memory. I had a try this, see the current version, it can pass all the test. However, I feel like it might be error-prone. The two shared fields have different way of initialization, currently I need to make sure `SampleContextFrames` is initialized then the ContextTrieNode field will be unknown status(not null). We might need to carefully use it in the future.
> 
> >reader and writer based on ProfileMap.
> > What is this usage?
> For CS profile, `SampleContext` is the key of ProfileMap, FullContext is always not empty here.  
> https://github.com/llvm/llvm-project/blob/main/llvm/lib/ProfileData/SampleProfReader.cpp#L279
> 
> 
> >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.
> Yeah, right now ProfileMap always require the `FullContext` as the key. wondering if we can only use FullContext as the key, FunctionSample's SampleContext doesn't own the fullContext.
> 
> 
Using union is error-prone, yes. My original thought is to use a separate map in ContextTracker that maintains the link from FunctionSamples to its ContextTrieNode. So far I don't see a use of getting ContextTrieNode from FunctionSamples out of the ContextTracker context. It seems encapsulating the map inside ContextTracker is enough. Does this make sense to you? 




================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:998
+  buildProfileMap();
+  // TODO: free the FunctionSamples' memory that is created in profile
+  // generator.
----------------
What are the allocations to be undone? I thought in D125246 we use std:move to transmit the FunctionSamples on the tri to profileMap. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127031