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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1085-1086
     if (!Samples) {
       InlinedGUIDs.insert(
           FunctionSamples::getGUID(CB->getCalledFunction()->getName()));
       return;
----------------
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?


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1083
+    // indirect call function to a direct call function, new callee target may
+    // not exist in the profile.
     if (!Samples) {
----------------
hoy wrote:
> Could mention more in the comment about that this is a problem with a stale profile.
> The popped out call instruction is different from what was pushed. 
> we only push the CALL instruction when we find its callee sample is not null to make sure we get a non-null samples when we pop the CALL. 

nit: make it explicit as to push and pop out of what.. here it's the inline candidate queue. 

I would say something like this:

In some rare cases, call instruction could be changed after being pushed into inline candidate queue, this is because earlier inlining may expose const-prop which can change indirect call to direct call. When this happens, we may fail to find matching function samples for the candidate later, even if a match was found when the candidate was enqueued. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154637