[PATCH] D154637: [SamplePGO] Fix ICE that callee samples returns null while finding import functions

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1085-1086
     if (!Samples) {
       InlinedGUIDs.insert(
           FunctionSamples::getGUID(CB->getCalledFunction()->getName()));
       return;
----------------
wlei wrote:
> wenlei wrote:
> > If it's not beneficial to inline/import (i.e. `getExternalInlineAdvisorShouldInline` returns false), should we just return without adding the callee to import candidate GUID set?
> Good question.
> I'm just reading the code of `getExternalInlineAdvisorShouldInline `, it looks to me `getExternalInlineAdvisorShouldInline ` return false doesn't mean `it's not beneficial to inlining/import`, it's only for replay, which is an uncommon situation, so most of time it return false, we still continue the importing.
> Say if we ignore the branch(line 1078), the question becomes : should we import a callee whose samples is null?
> 
> This actually didn't happen before, that's why there is an assertion. OK. but as this is an assertion, it seems it just means we shouldn't import it.
> 
> so the change can be simplified to change the assertion to a return, WDYT?
> 
> @hoy  I remember we discussed offline, you suggested to import it, otherwise it would miss the inlining opportunity for this call in post-link.
> 
> Also the below logic for non-null samples is to import the hot callee, since this is exposed by stale profile, so if the new function is hot, we have to import it.. but we don't know if the new function is hot or not...
> 
> 
> 
Say `bar` is just renamed to `baz`, we actually need to analyze `bar`'s samples to decide the import lists, but it's a function renaming matching which is a big topic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154637