Skip to content

Improve inlining "heavyweight" mode by excluding call sites #3085

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Sep 5, 2020

Conversation

MaxGraey
Copy link
Contributor

@MaxGraey MaxGraey commented Aug 29, 2020

This PR remove lightweight property from FunctionInfo and use two booleans hasCalls and hasLoops instead of it. It allows exclude functions with call sites. Otherwise potentially recursive functions will explode compilation time or/and code size.

Also rename allowHeavyweight to allowFunctionsWithLoops.

@MaxGraey MaxGraey changed the title Improve allowHeavyweight mode by excluding call sites Improve inlining "hevyweight" mode by excluding call sites Sep 1, 2020
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More generally, at the high level I think we need to decide on what the API should be here. And have specific reasons for why we think calls vs loops etc. matter.

As I said earlier, the existing logic was basically motivated by the fact that code without loops and without calls will run exactly once at most and as no surprising additional work happening. So it's fairly straightforward to estimate how much work will be done there. And specifically, to see that the work is likely very small, hence the inlining can help by removing call overhead to there.

So starting with a clear design about what you want to achieve here would be a good starting point I think @MaxGraey

@dcodeIO
Copy link
Contributor

dcodeIO commented Sep 1, 2020

As far as I understood the motivation for this change was that we are currently seeing Binaryen hanging forever when we try to compile the AssemblyScript compiler with allowHeavyweight=true, and Max figured that this is from recursively inlining heavyweight calls. As such, loops are fine, but calls are not. May well be that there is a better solution than just disabling calls, though.

@MaxGraey MaxGraey changed the title Improve inlining "hevyweight" mode by excluding call sites Improve inlining "heavyweight" mode by excluding call sites Sep 2, 2020
@MaxGraey
Copy link
Contributor Author

MaxGraey commented Sep 2, 2020

May well be that there is a better solution than just disabling calls, though.

inlining functions with calls should be avoided in any case, we could only enable / disable funcs with loops so current approach I think is fine)

@MaxGraey MaxGraey requested a review from kripken September 3, 2020 10:52
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks @MaxGraey and @dcodeIO

Yes, the infinite recursion issue with calls is a good reason to change this API. lgtm with some comment changes.

@kripken kripken merged commit 8b436ba into WebAssembly:master Sep 5, 2020
@MaxGraey MaxGraey deleted the improve-heavyweight-inlining branch September 5, 2020 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants