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

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.

In D112033#3079444 <https://reviews.llvm.org/D112033#3079444>, @modimo wrote:

> @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?

That sounds good to me. It makes the implementation cleaner. Setting threshold to 0 may lead to more importing than needed which can slow down build a bit, but I don't think that's a blocker for inline replay. Lgtm now.



================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1278-1281
+  if (!CalleeSamples &&
+      !(ExternalInlineAdvisor &&
+        ExternalInlineAdvisor->hasRemarksForFunction(
+            FunctionSamples::getCanonicalFnName(*CB->getCaller()))))
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112033