[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:63
+
+    // Add calls for context, if both caller and callee has context profile.
+    for (auto &Child : Caller->getAllChildContext()) {
----------------
hoy wrote:
> I'm wondering in the future if functions without profile should be considered so that a broader inline decisions can be made regardless of callsite hotness.
For pre-inlining, we need to have callee profile otherwise there's no profile to be adjusted regardless of whether we inline or not. 

For caller profile, we currently requires it to trigger pre-inline, but we could do pre-inline without caller profile. Is that what you meant? Currently compiler's inlining also requires caller profile, so what we have here aligns with compiler. 

(Not sure if I get the question, it's building top-down order here, not actual pre-inlining.. )


================
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:
> wenlei wrote:
> > hoy wrote:
> > > Nit: context
> > updated.
> Trying to understand how keys can change. Do we only remove the profiles from `ProfileMap` once they are merged into base profiles? Are base profiles from the reuse of first non-inlined profiles?
Right, base profile can be from the first promoted context profile. We don't remove profiles from ProfileMap during context promotion and merging. ProfileMap owns the function profiles, and context promotion is done on context trie only, which also updates context for the profiles. The key of that map does not change, but the context for profile in the map changes, so key is no longer accurate. Fortunately for profile writing, we don't look at the keys, so as long we fix this spot to avoid using the key, we are good. We could also update the map to keep keys accurate, but it involves moving profiles around which has some cost.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99146