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

Hongtao Yu via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Mar 14 22:03:01 PDT 2021


hoy added a comment.

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.

> 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 "
----------------
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`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98481



More information about the llvm-commits mailing list