Skip to content
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

Add no-inline IR annotation, and passes to set it based on function name #6146

Merged
merged 17 commits into from
Dec 6, 2023

Conversation

kripken
Copy link
Member

@kripken kripken commented Dec 5, 2023

Any function can now be annotated as not to be inlined fully (normally) or not to be
inlined partially. In the future we'll want to read those annotations from the proposed
wasm metadata section on code hints, and from wat text as well, but for now add
trivial passes that set those fields based on function name wildcards, e.g.:

--no-inline=*leave-alone* --inlining

That will not inline any function whose name contains "leave-alone" in the name.

--no-inline disables all inlining (full or partial) while --no-full-inline and
--no-partial-inline affect only full or partial inlining.

@kripken kripken requested a review from tlively December 5, 2023 00:25
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to mention that these flags take pattern arguments. Right now the help messages make it sound like they will apply to all functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

In general the help texts are short, see e.g. --extract-function which also doesn't mention its argument there. But if you run the pass you get more details:

$ wasm-opt --no-inline a.wat -all
Fatal: Usage usage:  wasm-opt --no-inline=WILDCARD

Maybe that's not enough, though?

Copy link
Member

Choose a reason for hiding this comment

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

Lgtm since it’s consistent and gives more detail when you try to use it.

}

// Otherwise, check if we can at least inline part of it, if we are
// interested in such things.
if (functionSplitter) {
if (!func->noPartialInline && functionSplitter) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Partial inlining may also trigger full inline. Should we guard against that when !func->noFullInline?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thanks! Yes, that logic needs to be handled. I fixed that now.

@kripken
Copy link
Member Author

kripken commented Dec 6, 2023

It occurred to me that we could make --no-inline work, rather than error as it does now, if we made it apply the flag to all functions. That seems like a natural behavior. But let's maybe wait to see if someone would actually want that.

@kripken kripken merged commit f722171 into main Dec 6, 2023
14 checks passed
@kripken kripken deleted the no-inline branch December 6, 2023 23:25
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
…ame (WebAssembly#6146)

Any function can now be annotated as not to be inlined fully (normally) or not to be
inlined partially. In the future we'll want to read those annotations from the proposed
wasm metadata section on code hints, and from wat text as well, but for now add
trivial passes that set those fields based on function name wildcards, e.g.:

--no-inline=*leave-alone* --inlining

That will not inline any function whose name contains "leave-alone" in the name.

--no-inline disables all inlining (full or partial) while --no-full-inline and
--no-partial-inline affect only full or partial inlining.
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