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

feat: add an alwaysInline builtin #2895

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CountBleck
Copy link
Member

Changes proposed in this pull request:
⯈ Add an alwaysInline builtin that inlines any inner function calls

  • I've read the contributing guidelines
  • I've added my name and email to the NOTICE file

This builtin operates similar to unchecked, by setting a new FlowFlag
that is checked in makeCallDirect in the area where the @inline
decorator is checked, thereby achieving the same functionality as it.
@CountBleck
Copy link
Member Author

@JairusSW let me know if anything else is desired for this builtin

Copy link
Member

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

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

what is the difference between inline and this builtin?

@CountBleck
Copy link
Member Author

@HerrCai0907 this gives you fine-grained control over when a function is inlined (by specifying it in the relevant call sites, instead of in the definition).

@JairusSW has an example as to how this would be useful in as-json.

@JairusSW
Copy link
Contributor

@CountBleck would it be possible to just rename it to inline()

@CountBleck
Copy link
Member Author

@CountBleck would it be possible to just rename it to inline()

I originally intended to name it inline, but I didn't want there to be a conflict with the @inline decorator in the TypeScript typings.

Copy link
Member

@HerrCai0907 HerrCai0907 left a comment

Choose a reason for hiding this comment

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

LGTM for implement part.
But I have some concern about design. I think we should avoid to pollute global scope. load / store / call_indirect can be mapped to low level wasm instruction directly so we keep them in global scope. But for alwaysInline and unchecked, I think we should place them in a special namespace or start with some special prefix.

For C++, std library used compiler-implemented function will start with __. some builtin function will start with __builtin. I wonder should we also do something like this?

e.g.
use this like

builtin_opt.unchecked(....)
builtin_opt.alwaysInline(....);

@CountBleck
Copy link
Member Author

Sounds like a good idea, but I'm hesitant to rename/move unchecked since that would be a breaking change.

@HerrCai0907
Copy link
Member

Sounds like a good idea, but I'm hesitant to rename/move unchecked since that would be a breaking change.

At least we can start from alwaysInline

@MaxGraey
Copy link
Member

Sounds like a good idea, but I'm hesitant to rename/move unchecked since that would be a breaking change.

How about to use inlined(fn(...)) instead of alwaysInline(fn(...)). It's shorter and consistent with unchecked

@CountBleck
Copy link
Member Author

I'll do that. What do you think about a namespace for inlined/unchecked? We can alias export const unchecked = builtin_opt.unchecked; (or whatever the namespace would be called).

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.

4 participants