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

[Wenlei He via Phabricator via llvm-commits]

wenlei 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"));
----------------
hoy wrote:
> nit: name the switch "use-context-cost-for-preinliner"?
changed


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


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:90
+    CurrNode = CurrNode->getChildContext(CallSiteLoc, CallerName);
+    if (CurrNode && CurrNode->getFunctionSize())
+      Size = CurrNode->getFunctionSize();
----------------
hoy wrote:
> `getFunctionSize` can be zero for function optimized away in the future? Wondering if `Optional` worker better here. 
Yeah, currently we don't track zero size context. This is one of the todos mentioned above. I plan to deal with that later when we properly track zero size - currently zero means unknown. 


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:97
+  // of inlinee under different context.
+  if (!Size && !CurrNode) {
+    CurrNode = PrevNode;
----------------
hoy wrote:
> 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`?
> if we don't find a valid size on the reverse tri `A @ B @ leaf`

You meant to say `B @ leaf`?

That's a good point though currently in this case, we won't find sibling `A @ C @ leaf` either, because the node for `B @ leaf` exists. We would get zero from `B @ leaf`. 

Assuming zero means unknown before we start to track zero size, we need to tweak it slightly to retrieve `A @ B @ leaf` in this case. 

```
  if (!Size) {
    if  (!CurrNode)
        CurrNode = PrevNode;
    while (!Size && CurrNode) {
      CurrNode = &CurrNode->getAllChildContext().begin()->second;
      Size = CurrNode->getFunctionSize();
    }
  }
```



================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:310
+        outs() << getReversedLocWithContext(
+            symbolize(IP, ShowCanonicalFnName, ShowPseudoProbe));
       }
----------------
hoy wrote:
> should use `UsePseudoProbes` instead of `ShowPseudoProbe`?
This was intentional as it though we might still want to have a way to dump line callsite even for binary with probes. I don't have a strong opinion though.


================
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 {
----------------
hoy wrote:
> typo: instructins
fixed


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108180