-
Notifications
You must be signed in to change notification settings - Fork 839
Intrinsic: js.called #8324
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
base: main
Are you sure you want to change the base?
Intrinsic: js.called #8324
Conversation
tlively
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % comments
| std::vector<Name> getConfigureAllFunctions(); | ||
|
|
||
| // Returns the names of all functions that are JS-called. That includes ones | ||
| // in configureAll (which we look through the module for), and also those |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still planning to eventually phase out this special handling for configureAll? I think we should to make the usage more consistent across different configureAll usage patterns.
| // configureAll functions are signature-called, and must also not be | ||
| // modified. | ||
| for (auto func : Intrinsics(*module).getConfigureAllFunctions()) { | ||
| // Signature-called functions must also not be modified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle we could still remove a suffix of the parameters, although I'm not sure whether that comes with a performance overhead. Maybe worth checking on that and adding a TODO if there would be no downside.
| // configureAll functions are signature-called, which means their params | ||
| // must not be refined. | ||
| for (auto func : Intrinsics(*module).getConfigureAllFunctions()) { | ||
| // Signature-called functions must not have params refined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to retire the "signature-called" terminology?
| ;; RUN: foreach %s %t wasm-opt --remove-unused-module-elements --closed-world -all -S -o - | filecheck %s | ||
|
|
||
| (module | ||
| (@binaryen.js.called) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the latest changes to the update script should put this annotation down with the function.
| @@ -0,0 +1,24 @@ | |||
| ;; RUN: wasm-opt -all %s -S -o - | filecheck %s | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can use the update script now.
When a function is annotated with this:
then it is assumed to be called from JS (if a reference is taken).
In closed world, we assume such references are not actually
called from the outside, and this annotation can be used to mark
the exceptions that are.
Such functions are not normally exported, but "js called" in
that JS may call them, but not other wasm code (so rec
group type identity does not matter).