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

[Di Mo via Phabricator via llvm-commits]

modimo added a comment.

@wenlei I refactored the implementation so that all queries to the external advisor route to `getExternalInlineAdvisorCost` or `getExternalInlineAdvisorShouldInline` depending on if we want the actual cost or just if inlining occurs, respectively. This unifies every location that queries for the existence of remarks (the original one in hasInlineAdvice and the new ones that add sites as candidates) and also means I don't need to expose a `hasRemarksForFunction` from the ReplayAdvisor and can do everything using the original InlineAdvisor interface. I also edited how importing is done:

1. If samples are not available then we can't transitively import but can at least import the symbol itself
2. If samples are present and external advisor wants to inline drop the import threshold to 0 so we have the opportunity to replay post-link with imported call sites

Does it make sense to drop the threshold to allow importing and are there any potential recursive issues with that?



================
Comment at: llvm/include/llvm/Analysis/ReplayInlineAdvisor.h:36
+    return (Scope == ReplayInlineScope::Module) ||
+           CallersToReplay.contains(F.getName());
+  }
----------------
wenlei wrote:
> 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.
> 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.

I looked into it more since the behavior is odd, when I query llvm-symbolizer on an address I get the .llvm. suffixes even though AFAIK it should use DWARF information:
```
llvm-symbolizer --inlining -e llvm-objdump 0x998400 --no-demangle
_ZL14getSegmentNamePKN4llvm6object15MachOObjectFileERKNS0_10SectionRefE.llvm.5974130588735922841
```

However, using GDB which directly queries dwarf:
```
(gdb) disas/s 0x998400
Dump of assembler code for function getSegmentName(llvm::object::MachOObjectFile const*, llvm::object::SectionRef const&):
```

Which doesn't contain any suffixes. Looking into it, llvm-symbolizer by default actually prefers using the symbol table over the dwarf information: https://reviews.llvm.org/D83530#inline-789802 and the option `--use-symbol-table` is now a no-op (D87067) so it always uses symbol table information. I manually edited `Opts.UseSymbolTable = false;` and confirm that the suffix does get removed by llvm-symbolizer.

What that means is that both sources I'm using (llvm-symbolizer and Rpass) will have these suffixes. Canonicalization isn't necessary though, implementing it when we need it makes sense rather than anticipating its usage. I don't particularly like the weird dependency of InlineAdvisor to `FunctionSamples` as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112033