[PATCH] D112033: [SampleProfile] Add all callsites to AllCandidates if InlineReplay is in effect

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ReplayInlineAdvisor.h:34
   bool areReplayRemarksLoaded() const { return HasReplayRemarks; }
+  bool hasRemarksForFunction(StringRef FuncName) const {
+    return (Scope == ReplayInlineScope::Module) ||
----------------
nit: the name is not accurate. it's more like `hasInlineAdvice`? When scope is module, this can return true while there's no remark for the function. 


================
Comment at: llvm/include/llvm/Analysis/ReplayInlineAdvisor.h:36
+    return (Scope == ReplayInlineScope::Module) ||
+           CallersToReplay.contains(F.getName());
+  }
----------------
hoy wrote:
> modimo wrote:
> > hoy wrote:
> > > Wondering if `CallersToReplay` could contain function names with those deduplicating suffixes, like `.llvm.` and if we should exclude those suffixes from `F.getName` using `FunctionSamples::getCanonicalFnName`. If the replay file from dwarf dump or Rpass?
> > Both dwarf (from llvm-symbolizer) and Rpass will emit `.llvm.` suffixes. Added getCanonicalFnName to uses.
> Thanks. How about move the `getCanonicalFnName` calls here?
How does this work with current version where uses have suffix stripped by getCanonicalFnName? CallersToReplay can still have the original names with suffixes from remarks so they won't match.

Moving `FunctionSamples::getCanonicalFnName` causes a weird dependency from InlineAdvisor to `FunctionSamples`. But not moving it here make things inconsistent between sample loader replay and cgscc replay. I actually don't think handling of suffix is critical for reply - consider the case with funique-internal-linkage-name. So we can leave out the canonicalization if possible. 

> Both dwarf (from llvm-symbolizer) and Rpass will emit .llvm. suffixes. 

I thought dwarf names won't have the llvm suffix, but only Rpass will have it since it's using symbol names which is changed by ThinLTO local to global promotion.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1088
+                ColdCandidates.push_back(CB);
+            } else if (ExternalInlineAdvisor &&
+                       ExternalInlineAdvisor->hasRemarksForFunction(
----------------
Can we move the following into a helper SampleProfileLoader::ExternalInlineAdviceAvailableForFunction(..), it's used in a few places.

```
ExternalInlineAdvisor &&
    ExternalInlineAdvisor->hasRemarksForFunction(F)
```


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1143
+            ExternalInlineAdvisor->hasRemarksForFunction(FunctionSamples::getCanonicalFnName(F))) {
+          if (shouldInlineCandidate(Candidate))
+            InlinedGUIDs.insert(
----------------
When no recommendation is available from replay advisor, this should be a no-op. However with the way it's currently implemented, this will actually fall back to run call analyzer for regular inline decision in `shouldInlineCandidate`? May the checks above guarantees the advice will always be available here, but the way it's setup seems a bit fragile and not fail-safe.. 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1144-1145
+          if (shouldInlineCandidate(Candidate))
+            InlinedGUIDs.insert(
+                FunctionSamples::getGUID(I->getCalledFunction()->getName()));
+        } else {
----------------
There're two issues:

- If we rely on this path to influence importing using replay input, we would lose that influence completely for non-FDO path, but replay is not meant for FDO only. 
- On the other hand, for FDO, we have the opportunity to look deeper into the call tree and do better, i.e following `findExternalInlineCandidate` to import lower level candidates too.  

For #2, with current implementation, even if the inline replay provides the exact same decision as the native inline decision, we could miss out inlining due to missing importing here. 

And this added complexity logically still belongs to `findExternalInlineCandidate`, I suggest we move the logic there to keep the main flow cleaner here. 

All above applies to `inlineHotFunctionsWithPriority` as well.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1278-1281
+  if (!CalleeSamples &&
+      !(ExternalInlineAdvisor &&
+        ExternalInlineAdvisor->hasRemarksForFunction(
+            FunctionSamples::getCanonicalFnName(*CB->getCaller()))))
----------------
Add a comment please.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1879
   if (FAM && !ProfileInlineReplayFile.empty()) {
-    ExternalInlineAdvisor = getReplayInlineAdvisor(
+    ExternalInlineAdvisor = std::make_unique<ReplayInlineAdvisor>(
         M, *FAM, Ctx, /*OriginalAdvisor=*/nullptr, ProfileInlineReplayFile,
----------------
With the only callsite for `getReplayInlineAdvisor` removed, the function and all stuff around `HasReplayRemarks` can be removed too? We now assume ReplayInlineAdvisor always have remarks loaded, right? 


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1095
       }
       if (Hot || ExternalInlineAdvisor) {
         CIS.insert(CIS.begin(), AllCandidates.begin(), AllCandidates.end());
----------------
hoy wrote:
> modimo wrote:
> > hoy wrote:
> > > Nit: This code doesn't exist on the `inlineHotFunctionsWithPriority` path which mainly serves csspgo. We should bring up the support there, could be in a separate change.
> > Makes sense. I added the code path but not sure how to generate a csspgo profile for function_metadata.ll to test it out. Is there a simple way to do that or should I create a test case from scratch?
> Thanks for working on the csspgo support. To test thinlto function import, here is an existing test case 
>  llvm/test/Transforms/SampleProfile/csspgo-import-list.ll. It can be mocked to take in a replay file. I'm fine with make a separate change for csspgo work.
Good point Hongtao. I think it's better if we include the change for `inlineHotFunctionsWithPriority` in this patch - it feels incomplete otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112033