[PATCH] D125246: [CSSPGO][llvm-profgen] Reimplement CS profile generator using context trie

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:941
+    NewProfile.setContext(FContext);
+    delete (FProfile);
+  }
----------------
wlei wrote:
> hoy wrote:
> > Can `FProfile` be updated in place to avoid copy and deletion?
> the profile in ProfileMap require the full context as the key, so we can't create the value in `ProfileMap` in the early time. Or perhaps we can use the move semantics, but `FunctionSamples` currently doesn't have the move constructor, which will make things complicated. I think after the trimming and pre-inliner, the num of profile will be small, copy and deletion should be fine then?
Makes sense. Eventually the trie should be small after preinliner.

You may want to set `Node.getFunctionSamples()` to be null after deleting it.




================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:954
+  SampleContextFrameVector Context;
+  buildProfileMapFromContextTrie(&RootContext, Context);
+
----------------
wlei wrote:
> wlei wrote:
> > hoy wrote:
> > > hoy wrote:
> > > > I guess eventually this call will be after the preiliner run, but can this be moved after `computeSummaryAndThreshold` now? 
> > > nit: seems that `Context` is not used anywhere. Perhaps make a wrapper function `void buildProfileMapFromContextTrie(    ContextTrieNode *Node)` and declare `Context` inside? 
> > Sounds good!
> > I guess eventually this call will be after the preiliner run, but can this be moved after computeSummaryAndThreshold now?
> 
> Yeah, we need to move it after preinliner and moreover it should be after `SampleContextTrimmer`.  
> 
>  `computeSummaryAndThreshold` is still using `ProfileMap` and I also found that `ProfileMap` can also be imported from the SampleReader. Both of them will needs to be rewritten. Maybe leave them a separate diff, just to be easy for the code review, what do you think?
> computeSummaryAndThreshold is still using ProfileMap

I'm wondering if we can make a version of computeSummaryAndThreshold to run on the trie to avoid generate a giant ProfileMap with all contexts in the middle. 

SampleReader is a special path and we don't care about the performance there. It can be treated differently.

Yeah, a separate diff is fine.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125246