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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

Add a test case for the inlining (non-importing) path? to the existing test llvm/test/Transforms/SampleProfile/inline-replay.ll ?



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


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1095
       }
       if (Hot || ExternalInlineAdvisor) {
         CIS.insert(CIS.begin(), AllCandidates.begin(), AllCandidates.end());
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112033