wabt icon indicating copy to clipboard operation
wabt copied to clipboard

wasm2c: add support for module instancing

Open yhdengh opened this issue 3 years ago • 7 comments
trafficstars

This PR adds support for module instancing to wasm2c as proposed in #1805. The approach is similar to #1721.

  • Globals, memories, and tables are moved into a new structure named module_prefix_ + _instance_t.
  • The structure is passed as the first argument of function calls. The embedder is responsible for initializing the structure.
  • run-spec-wasm2c.py is updated such that main.c generated by the script reflects the changes for module instancing.

~~(As a result of these changes, it is no longer possible to run wasm2c on different modules independently and later link the outputs together.)~~

~~(This PR is sequenced behind #1875.)~~

yhdengh avatar Jan 26 '22 01:01 yhdengh

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

shravanrn avatar Feb 03 '22 15:02 shravanrn

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 https://github.com/WebAssembly/wabt/pull/1853#issuecomment-1062094906 .

keithw avatar Mar 30 '22 02:03 keithw

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.

shravanrn avatar Apr 05 '22 03:04 shravanrn

@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 (https://github.com/WebAssembly/wabt/pull/1877#pullrequestreview-931162689) 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.

keithw avatar Apr 06 '22 23:04 keithw

Can you rebase now that #1897 and #1896 have landed? I hope it should make this change much more precise.

sbc100 avatar Apr 14 '22 15:04 sbc100

Can you rebase now that #1897 and #1896 have landed? I hope it should make this change much more precise.

Here you go.

keithw avatar Apr 14 '22 23:04 keithw

@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.

Having a look now. Sorry for the delay

shravanrn avatar Apr 18 '22 18:04 shravanrn

Ok! Marking as auto-merge!

sbc100 avatar Sep 16 '22 16:09 sbc100

OK I think this is ready to land now. Any final changes before we do so?

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:

  1. Bulk memory ops (#1877)
  2. Reference types (#1887) [at this point, wasm2c will pass all the current Wasm tests (+ multi-memory and exceptions) except SIMD, and it would be cool to get it added to https://webassembly.org/roadmap/]

Followed by more speculative changes:

  • Changes necessary to accommodate Firefox/RLBox (we chatted a bit with talg a few weeks ago)
  • SIMD [at this point, wasm2c would support all of WebAssembly 2.0 + multi-memory and exceptions]
  • Adding an instance "reset" function (in addition to "instantiate" and "free") to allow the host to to take an instance and more efficiently restore it to initial state without having to free and reallocate all its memories and tables.
  • Adding a mechanism (maybe in the IR or in the CWriter options) to tag memories on a memory-by-memory basis for whether they should be hardware bounds-checked (with mprotect and a signal handler) or software bounds-checked (with explicit checks on every load/store), rather than making this a global choice for all memories. We're finding this useful because we generally want memory#0 to be hardware checked (for performance) but we also like being able to point the other memories at arbitrary places without having to set every one up beforehand with mmap/mprotect. Not sure this is going to be of general interest.
  • ...?

keithw avatar Sep 16 '22 16:09 keithw