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

Fri Jul 7 11:08:57 PDT 2023

hoy added inline comments.

================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1085-1086
     if (!Samples) {
       InlinedGUIDs.insert(
           FunctionSamples::getGUID(CB->getCalledFunction()->getName()));
       return;
----------------
wenlei wrote:
> hoy wrote:
> > wenlei wrote:
> > > hoy wrote:
> > > > hoy wrote:
> > > > > wenlei wrote:
> > > > > > hoy wrote:
> > > > > > > wlei wrote:
> > > > > > > > wenlei wrote:
> > > > > > > > > hoy wrote:
> > > > > > > > > > wenlei wrote:
> > > > > > > > > > > wlei wrote:
> > > > > > > > > > > > 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.
> > > > > > > > > > > Lei, I think you're right. Before replay inline was added (D112033), there was only this assertion. I think it makes sense to just return when Samples is null instead of assert non-null, with the explanation of the case we ran into. This should be really rare, so perf wise it probably won't matter import or not. 
> > > > > > > > > > Importing it is probably not that beneficial but it matches the behavior of in-module inlining.
> > > > > > > > > > it matches the behavior of in-module inlining. 
> > > > > > > > > 
> > > > > > > > > Being in the inline candidate queue doesn't mean it will be inlined if it's not external. It only means it will be evaluated. The actual inline decision is in tryInlineCandidate->shouldInlineCandidate (for direct call), and at this point there is no guarantee that it would be inlined if it's not external.
> > > > > > > > > Importing it is probably not that beneficial but it matches the behavior of in-module inlining.
> > > > > > > > 
> > > > > > > > @hoy  On a second thought, speaking of "matches the behavior of in-module inlining.", we also don't inline a callee function whose samples is mismatched/null, as this is "sample profile inlining". If it's other inliner(CGSCC) inline it, other inliner should do the importing.
> > > > > > > > 
> > > > > > > > So maybe just returning null make more sense?
> > > > > > > Right, importing just offers an opportunity for it to be inlined. Both in-module and external inlining have to go through the inliner's cost model once the IR is ready.
> > > > > > I was just confused by what you meant "matches the behavior of in-module inlining".
> > > > > > 
> > > > > > For local functions, it's available, and we evaluate every function calls. For external functions, we can't possibly make all functions available like local functions, so we have to choose to import those that are likely to be inlined based on hotness. 
> > > > > > 
> > > > > > If we import everything regardless of hotness, that would "matches the behavior of in-module inlining", but that's not practical, and not what we do today. 
> > > > > > 
> > > > > By matching the behavior of in-module inlining, I mean the case of indirect call promoted to a direct local callee, in which case the promoted callee will be considered inlining without going through a sample lookup:
> > > > > 
> > > > > https://github.com/llvm/clangir/blob/b7d9322b4963e620dfd12246816e6f7b2da5fd88/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1500
> > > > > 
> > > > > In our case, the callee is external. I think it should be imported and go through the same logic above.
> > > > Oh, there's a subtle difference here. Even if the callee is imported, it'll still not considered an inline candidate because of the sample lookup error in postlink. If the callsite has branch metadata, the callee may be inlined by postlink CGSCC. But that's not a match of sample profile inliner. So returning null is more reasonable. 
> > > > indirect call promoted to a direct local callee, in which case the promoted callee will be considered inlining without going through a sample lookup 
> > > 
> > > This is different - the promoted call still go through hotness evaluation (tryPromoteAndInlineCandidate->tryInlineCandidate->shouldInlineCandidate). 
> > > 
> > > The question is, whether we filter on hotness before we decided to import, the answer is yes, and here we don't have hotness, so we should not import.
> > > 
> > >  
> > An alternative as discussed offline, place a sample lookup check right after popping an candiate. This should line up in-module and external inlining for our case:
> > 
> > https://github.com/llvm/clangir/blob/b7d9322b4963e620dfd12246816e6f7b2da5fd88/llvm/lib/Transforms/IPO/SampleProfile.cpp#L1443 
> > 
> > But it sounds a bit over-engineering as the case isn't common, and matching behavior isn't the demanding.
> > matching behavior 
> 
> I think "sample lookup" is a detail that is not important to match. If we have to match, I'd argue to match the check on hotness before inlining/importing - in which case, we shouldn't import here. 
> This is different - the promoted call still go through hotness evaluation (tryPromoteAndInlineCandidate->tryInlineCandidate->shouldInlineCandidate).

I was thinking the imported callee can still go though this but just realized it won't even be considered a candidate as it doesn't have the nested callee profile anywhere.

> The question is, whether we filter on hotness before we decided to import, the answer is yes, and here we don't have hotness, so we should not import.

I think given a stale profile, the current behavior is to accidentally trust the hotness in the profile and let it drive the inlining for a promoted icall. If we don't want this, we should consider the alternative above.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154637