-
-
Notifications
You must be signed in to change notification settings - Fork 121
feat(compiler): Use Binaryen's bulk memory polyfill #2334
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): Use Binaryen's bulk memory polyfill #2334
Conversation
eef1b53 to
db1f96f
Compare
db1f96f to
e659c4b
Compare
| [ | ||
| ("source", Builtin_types.type_wasmi32), | ||
| ("destination", Builtin_types.type_wasmi32), | ||
| ("dest", Builtin_types.type_wasmi32), |
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 noticed these signatures were slightly wrong previously, we never saw this behaviour because the names ended up coming from the polyfills rather than from the primitives however without correcting this mistake this pr would be breaking if you tried to use named arguments.
e659c4b to
7ecd783
Compare
ospencer
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.
I do think we need some kind of smoke test here. We don't need to test Binaryen's behavior in the sense that we need to make sure that the polyfill is correct, but we do need something that makes sure if those couple lines of code get deleted the test fails.
An idea: parse the finished module and verify that the bulk memory feature isn't on.
7ecd783 to
8290f80
Compare
|
Made those changes |
|
And the smoke test? |
~~I don't think that this would be trivial; it would basically be implementing a WASM parser. It could be a little simpler, we could scan the code section for the instruction codes instead of parsing, but we would need to ensure they aren't showing up elsewhere ever do to any other changes, such as in an If we're really serious about this, we could maybe use binaryen to load the code back in and check it with that, but I don't think that's the most trivial thing either.~~ Nevermind I understand what you mean now. |
8290f80 to
81b5028
Compare
|
Some tests added. |
81b5028 to
54bffa7
Compare
|
Addressed those comments. I export the |
This replaces our custom bulk memory polyfills to use the binaryen pass implementation. Notes: * We no longer test this behaviour as we have no way to test wasm output and we are not trying to test binaryen * We leave the feature flag on now as the instructions get optimized out after and if its off it causes issues. * Bulk memory is now supported across the board in runtimes so its far less of a concern then it was initially. Draft: This is a draft as we need to merge the binaryen 124 stuff before we can merge this Closes: grain-lang#2330
54bffa7 to
35fdd78
Compare
ospencer
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.
Thanks, looks great!
This replaces our custom bulk memory polyfills to use the binaryen pass implementation.
Notes:
Closes: #2330