[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#2629511 <https://reviews.llvm.org/D98481#2629511>, @hoy wrote:

> In D98481#2628143 <https://reviews.llvm.org/D98481#2628143>, @davidxl wrote:
>
>> Without guarding the caller size, this (over topdown inlining) can also potentially happen without recursion.
>>
>> This patch seems too aggressive. One possible way to handle it is to restrict inlining to a node N in non trivial SCC when node N has >1 fan-out (edges) to other nodes in the same SCC -- the large fan-out is the reason for the excessive growth.
>
> Thanks for the suggestion. Exactly, the excessive growth boils down to the fan-out. An additional check for the fan-out factor should effectively make the restriction less aggressive.

Fan-out is one way to control the growth, I'm not sure if we can make it narrow and selective enough though.

I also wanted to point out that this case is related to the use of a partial profile, in which case the hot threshold may have triggered more aggressive inlining.

Actually we have a long list of issues where the CGSCC inliner explodes (w/ or w/o profile). It seem to me that the way CGSCC inliner works relies heavily on a "good" threshold, otherwise there's this risk of exponential growth of size and build time regardless of whether we have recursion or not just like David pointed. We indeed have seen cases where the default inline threshold leads to excessive inlining when there's no recursion.

While in theory if we have a reasonable threshold, CGSCC inliner would finish in reasonable amount of time (unless we have bugs that cause SCC mutation to not converge), it's hard for a preset threshold to fit all builds, which is why we ran into such cases again and again. I know that there's past discussion about introducing size/growth cap for CGSCC inliner and it didn't go through, but I'm wondering if we should revisit that given that preset threshold lead to explosive inlining in quite a few cases. We can find way to get around it by tuning threshold, but it's questionable to me whether the time spent is worthwhile to chase down these cases one by one and ending up tuning threshold without finding fundamental issues other than CGGSCC inliner is unbounded and rely on threshold.

If we cap size, we would need to inline more important call sites first. At the minimum, can we add a switch (off by default) to cap size so when people run into this, there would offer a very targeted solution - currently we sometimes have to tune down threshold globally, which negatively affects other inline paths too. We can also add a warning when inliner is throttled due to cap.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98481