[PATCH] D108180: [CSSPGO] Track and use context-sensitive post-optimization function size to drive global pre-inliner in llvm-profgen

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/CSPreInliner.cpp:35
+cl::opt<bool> UseContextCostForPreInliner(
+    "context-cost-for-preinliner", cl::Hidden, cl::init(false),
+    cl::desc("Use context-sensitive byte size cost for preinliner decisions"));
----------------
nit: name the switch "use-context-cost-for-preinliner"?


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:416
+  if (EnableCSPreInliner) {
+    assert(BinarySampleCounters.size() == 1);
+    ProfiledBinary &Binary = *BinarySampleCounters.begin()->first;
----------------
nit: this is gone with D108002


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:90
+    CurrNode = CurrNode->getChildContext(CallSiteLoc, CallerName);
+    if (CurrNode && CurrNode->getFunctionSize())
+      Size = CurrNode->getFunctionSize();
----------------
`getFunctionSize` can be zero for function optimized away in the future? Wondering if `Optional` worker better here. 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:97
+  // of inlinee under different context.
+  if (!Size && !CurrNode) {
+    CurrNode = PrevNode;
----------------
When `Size` is invalid, can it mean `Context` is shorter than an inline path in previous build? For example, given a leaf function with a previous inline path  `A @ B @ leaf`, and the current context `B @ leaf`, if we don't find a valid size on the reverse tri `A @ B @ leaf` , should we fall back to use the size stored in node `A`, instead of finding a sibling context like `A @ C @ leaf`?


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:310
+        outs() << getReversedLocWithContext(
+            symbolize(IP, ShowCanonicalFnName, ShowPseudoProbe));
       }
----------------
should use `UsePseudoProbes` instead of `ShowPseudoProbe`?


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:109
+// have instructions. To fix this, we need to mark all inlinee with entry probe
+// but without instructins as having zero size.
+class BinarySizeContextTracker {
----------------
typo: instructins


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108180