[PATCH] D118515: [llvm-profgen] On-demand track optimized-away inlinees for preinliner.

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:608-609
+
+  // Flush the symbolizer to save memory.
+  Binary->flushSymbolizer();
 }
----------------
It is weird to have profile generator flush symbolizer of ProfileBinary, how much saving is this comparing to on-demand context size tracking?  

I assume the symbolization here is only needed for dwarf base profile, can we free symbolizer for probe profile generation right after ProfiledBinary::load? 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:351
+  if (UsePseudoProbes && TrackFuncContextSize) {
+    for (const auto &Child : ProbeDecoder.getDummyInlineRoot().getChildren()) {
+      auto *Frame = Child.second.get();
----------------
Actually ProbeDecoder.getDummyInlineRoot().getChildren() is already a map, wondering can we make it possible to look up by function name without building an extra map? 

I can see that the key is Guid, ProbeId pair - we can get Guid from names, but ProbeId under dummy root is index. What was the reason for top level nodes to have different probe Id instead of a dummy probe Id? For top level, we don't expect same name to appear more than once. 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:483
 
+  void flushSymbolizer() { Symbolizer->flush(); }
+
----------------
If this invalidates symbolizer, i.e making it not functional, we should also reset the pointer to null.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118515