-
Notifications
You must be signed in to change notification settings - Fork 725
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
wasm2c: add support for module instancing #1814
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.
Overall this looks good to me, but I have a few questions.
@shravanrn please take a look as well. Thanks!
Reviewing! Although its getting a bit late in my local timezone (I am currently traveling), so worst case I'll post an update in the next 12 hours or so |
815d34f
to
c15831c
Compare
c15831c
to
81d5ca4
Compare
5225d1e
to
3e5519e
Compare
3e5519e
to
55dd0a5
Compare
This now passes the current wasm2c tests, including for imports/exports/linking (these are the "old" spec tests from before bulk-memory operations and reference-types were merged). We're still working on implementing bulk-memory operations and reference-types on top of this, which if successful will hopefully have wasm2c passing the current versions of the spec tests, as well as the remaining multi-memory proposal tests that depend on these features. I've been tracking this at #1853 (comment) . |
9f40f48
to
cf8d31d
Compare
TLDR: we can go ahead and land this instancing support when ready. I'll deal with the incompatibilities they may introduce in our fork @kripken @sbc100 Just wanted to briefly update everyone. I was actually away for the last few weeks hence the delayed response here. But I did play a bit more with rebasing some of these changes on our wasm2c fork. In short it should be possible with some work. That said, my current priority for Mozilla is to rewrite RLBox to add more features, so I suspect I won't be able to upstream some of our custom wasm2c features in our fork for a few more weeks. However, I don't want this to block any other ongoing work here, so feel free to land this instancing support. I am happy however to review this and any other PRs you want me to look at. |
cf8d31d
to
2e8f1bc
Compare
@shravanrn Thanks, we appreciate this. Of course we'd be grateful for your review. When you do want to upstream the rlbox code, we're happy to help rebase it at the time (if you want our help) and preserve whatever tests are passing. I don't think the two approaches are that different; ours is mostly the same as yours + handling imports/exports/linking, cases that are probably less relevant to the RLBox use-case. @sbc100 @kripken This is probably ready for another look. @binji gave us a review here (#1877 (review)) that we've since responded to. After this one, the later-stacked PRs are #1877 (bulk memory), #1887 (reference types), and #1888 (tolerating partly invalid modules), after which wasm2c passes all the core spec tests (and all the multi-memory proposal tests) except the SIMD ones. |
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 created a separate PR to remove the signature mangling: #1896. Perhaps we can land that first to simplify this PR?
f262c76
to
0f66158
Compare
881a4dd
to
bf69a30
Compare
b12ba7e
to
56f4441
Compare
56f4441
to
f8b5cef
Compare
e59ec68
to
efc90be
Compare
efc90be
to
aaafc01
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.
OK I think this is ready to land now. Any final changes before we do so?
Index memory_index = 0; | ||
for (const Memory* memory : module_->memories) { | ||
bool is_import = memory_index < module_->num_memory_imports; | ||
if (!is_import) { | ||
Write("static "); |
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.
Shouldn't this be static is the memory is not exported?
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.
These memories are getting declared as members of the instance struct (both the exported and not-exported ones) so I don't think static
is needed/valid anymore in that new context.
Co-authored-by: Angela Montemayor <amontema@cs.stanford.edu>
aaafc01
to
6ed3e90
Compare
Ok! Marking as auto-merge! |
Awesome! I don't think we have any final changes in this one. I just rebased it on the current tip-of-tree to make sure it applies cleanly and then rebased #1877 and #1887 on top of it. I think after this the wasm2c roadmap from our perspective looks something like:
Followed by more speculative changes:
|
Co-authored-by: Angela Montemayor <amontema@cs.stanford.edu>
This PR adds support for module instancing to wasm2c as proposed in #1805. The approach is similar to #1721.
module_prefix_ + _instance_t
.run-spec-wasm2c.py
is updated such thatmain.c
generated by the script reflects the changes for module instancing.