[PATCH] D99146: [CSSPGO][llvm-profgen] Context-sensitive global pre-inliner

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:105
+
+  // Ideally we want to consider everything a functions calls, but as far as
+  // context profile is concerned, only those frames that are children of
----------------
hoy wrote:
> nit: a function call
updated.


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:150
+  unsigned int SampleThreshold = SampleColdCallSiteThreshold;
+  if (Candidate.CallsiteCount > HotCountThreshold)
+    SampleThreshold = SampleHotCallSiteThreshold;
----------------
hoy wrote:
> Should this be `>=`?
`>` is consistent with `SampleProfileLoader::shouldInlineCandidate`, though practically I don't think it matters. 


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:168
+  // Use the number of lines/probes as proxy for function size for now.
+  unsigned FuncSize = FSamples->getBodySamples().size();
+  unsigned FuncFinalSize = FuncSize;
----------------
hoy wrote:
> This currently only reflects the number of live/hot lines. Might be extended to using static size from dwarf/probe decoding or disassembling. Can you leave a TODO for this? 
Todo added.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:470
+    // After CSPreInliner the key of ProfileMap is no longer accurate for
+    // contex, use the context attached to function samples instead.
+    std::string ContextWithBracket =
----------------
hoy wrote:
> Nit: context
updated.


================
Comment at: llvm/tools/llvm-profgen/ProfiledCallGraph.h:29
+
+  struct ProfiledCallGraphNodeComparor {
+    bool operator()(const ProfiledCallGraphNode *L,
----------------
hoy wrote:
> Nit: ProfiledCallGraphNodeComparer
Good catch, changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99146