-
-
Notifications
You must be signed in to change notification settings - Fork 113
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(compiler): Call known functions across module boundaries #1175
Conversation
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.
Still wrapping my head around this, but I've left some initial thoughts
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.
Wow, that's a lot of work. I'm not going to spot problems, but I can say the approach looks good to me.
7447bb8
to
19e7a76
Compare
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 seems good IMO. One question before merging, but that's it.
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.
Snapshots look great!! I had a few questions/comments.
mimp_used: true, | ||
}, | ||
], | ||
) | ||
| FunctionShape(_) => |
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.
Can you explain FunctionShape to me a bit? Since it seems like we're adding support that we didn't have before
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.
Of course! Imports take one of two shapes, GlobalShape
or FunctionShape
, corresponding to wasm globals or wasm functions. Imports that happen via foreign wasm
are either GlobalShape
or FunctionShape
, and before this PR, Grain imports were only GlobalShape
, meaning that imported functions were only ever imported as values—the closure for the function. This meant that the only way the function could be called was via a call_indirect
.
This change allows us say that we're importing an actual Grain webassembly function (and its closure), so we can call it via a regular call
instruction rather than having to pull the pointer from the closure.
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.
Excellent! ❤️
This PR uses regular
call
s where possible between modules. This should be fairly significant for performance, as directcall
s are much faster thancall_indirect
s. Additionally, Binaryen can now provide better optimizations after linking.