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

[Di Mo via Phabricator via llvm-commits]

modimo added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1278-1281
+  if (!CalleeSamples &&
+      !(ExternalInlineAdvisor &&
+        ExternalInlineAdvisor->hasRemarksForFunction(
+            FunctionSamples::getCanonicalFnName(*CB->getCaller()))))
----------------
wenlei wrote:
> wenlei wrote:
> > Add a comment please.
> Maybe you missed this one, a comment will be helpful. When sample loader inliner is used for reply, we want to inline callee if replay advisor say so even when the callee doesn't have profile.
> 
> One thing I'm not exactly sure is how robust is the sample loader inliner for handling candidate without a profile. For testing, maybe you want to try removing the check altogether for a regular (non-replay) build for a larger component, and see if there's any trouble.
> Maybe you missed this one, a comment will be helpful. When sample loader inliner is used for reply, we want to inline callee if replay advisor say so even when the callee doesn't have profile.
Added
> One thing I'm not exactly sure is how robust is the sample loader inliner for handling candidate without a profile. For testing, maybe you want to try removing the check altogether for a regular (non-replay) build for a larger component, and see if there's any trouble.
That's a good idea to validate robustness, I'm run with this check removed on CSSPGO clang build and see what shakes loose.



================
Comment at: llvm/test/Transforms/SampleProfile/Inputs/csspgo-import-list.prof:3
  2: 12
- 3: 18 _Z5funcAi:11
+ 2.1: 18 _Z5funcAi:11
  3.1: 18 _Z5funcBi:19
----------------
FYI I was debugging through this and found that sample associated with CallBase expected a discriminator for _Z5funcAi. I'm assuming these were meant to fully match against the source. I added csspgo-import-list-no-funca.prof as an explicit test case where _Z5funcAi samples are missing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112033