[PATCH] D98481: [Inliner] Do not inline mutual-recursive functions to avoid exponential size growth.

[Wenlei He via Phabricator via llvm-commits]

wenlei added a comment.

In D98481#2625282 <https://reviews.llvm.org/D98481#2625282>, @hoy wrote:

> In D98481#2624371 <https://reviews.llvm.org/D98481#2624371>, @wenlei wrote:
>
>> Thanks for looking into this. This fix is more restrictive than I originally thought. And I'm also not sure if it does what we needed (see inline comment for this part).
>
> Right, it doesn't do exactly what we need, which is a global inline history to prevent a recursive callsites being inlined repeated It aggressively prevents inlining recursive callsites out of their defining SCCs.
>
>> IIUC, if we have A->B, B->C, C->B, even if there's no inlining between B and C, with this change, A will never inline B. Is that correct? I imagine that would throttle inliner very visibly - we can gauge the effect by looking at the number of inlining w/ and w/o this change for spec and larger programs.
>
> Correct, with this change `B` and `C` will never be inlined into any function outside their enclosing SCC. I agree this is aggressive. The effect could be propagated as the bottom-up walk goes.

Practically, it means this change can potentially have negative performance impact on many workloads, which by itself would definitely be a deal breaker. And if that's not the case, we need to prove it through testing with this change. (Even if we don't observe any negative impact on a few workloads, it's still not bullet proof, but it's at least a data point).

>> Ideally, we don't want to limit all inlining across SCC if the inlinee is part of a recursion cycle. The problem here is the repeated inlining due to recursion. We have `InlineHistory` for single iteration SCC run, and `InlinedInternalEdges` for cross iteration checks against repeated recursive inlining in the same SCC, but they don't restrict all recursive inlining in general. It would be good to have something that only limits the extra, repeated inlining for this case too.
>>
>> I think `InlinedInternalEdges` is close to what is needed here, except that here we're dealing with different SCC when moving up the SCC graph. Wondering if we can somehow transfer the `InlinedInternalEdges` of previously processed SCC so later SCC is also aware of what would be repeated recursive inlining, though as @chandlerc called out `InlinedInternalEdges` itself is already a hack that breaks layering.
>>
>> + at chandlerc, and @wmi who improved `InlinedInternalEdges` in https://reviews.llvm.org/D52915.
>
> A global inline history across also SCCs sounds good fit there, however, building such a history is a bit challenging it that how to select the hash keys. It would be good to know if `InlinedInternalEdges` can be extended to handle repeatedly inlining a recursive callsite across SCCs.





================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:833
+      // is handled by inline cost analyzer.
+      if (CalleeSCC != C && CalleeSCC->size() > 1) {
+        LLVM_DEBUG(dbgs() << "Skipping inlining a node that is involved in a "
----------------
hoy wrote:
> wenlei wrote:
> > If the goal is to prevent repeated recursive inlining when processing caller SCC, this check may not always work either depending on what state we are with (A,B) SCC. 
> > 
> > If we stop at A inlined B (so A only calls A, but not B), then the (A,B) SCC is broken into (A) and (B), in which case the CalleeSCC size check against 1 may pass when we look at C or D. So what happens could be - we pass this check at caller C, doing more inlining, then fail this check at D, bouncing back and a forth as we move up.
> Do you mean that if `A` turns into self-recursive, `C` then can inline it because `CalleeSCC->size()  == 1`?
Right. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98481