binaryen icon indicating copy to clipboard operation
binaryen copied to clipboard

Mutli-Memories Support in IR

Open ashleynh opened this issue 2 years ago • 3 comments

Multiple Memories for Wasm Proposal

This PR removes the single memory restriction in IR, adding support for a single module to reference multiple memories. To support this change, a new memory name field was added to 13 memory instructions in order to identify the memory for the instruction.

It is a goal of this PR to maintain backwards compatibility with existing text and binary wasm modules, so memory indexes remain optional for memory instructions. Similarly, the JS API makes assumptions about which memory is intended when only one memory is present in the module. Another goal of this PR is that existing tests behavior be unaffected. That said, tests must now explicitly define a memory before invoking memory instructions or exporting a memory, and memory names are now printed for each memory instruction in the text format.

There remain quite a few places where a hardcoded reference to the first memory persist (memory flattening, for example, will return early if more than one memory is present in the module). Many of these call-sites, particularly within passes, will require us to rethink how the optimization works in a multi-memories world. Other call-sites may necessitate more invasive code restructuring to fully convert away from relying on a globally available, single memory pointer.

ashleynh avatar Jul 18 '22 16:07 ashleynh

Would love to see this feature! We can really use it at WasmEdge. 😂 Please let us know how we can be of assistance.

juntao avatar Jul 26 '22 09:07 juntao

🎉 🎉 🎉

tlively avatar Jul 29 '22 20:07 tlively

I would be ok with landing this as-is and applying fixups in follow-on PRs after the fuzzer passes :+1:

Edit: It might be useful to first update printing to elide names where possible, though, to minimize test churn.

tlively avatar Aug 04 '22 21:08 tlively

Looks like there are some new test files here, test/multi-memories*? It would be good to make those wast files instead of wasm, unless I'm missing a reason not to? As text they are easier to review, and look at if a test fails in the future.

@kripken I added the multi-memories wasm test files to verify correct parsing of binary format and printing to text format. There are additional ".wasm.fromBinary" text files added for each binary to verify the wast is as expected. Happy to make any change needed here.

ashleynh avatar Aug 17 '22 18:08 ashleynh

@ashleynh

I added the multi-memories wasm test files to verify correct parsing of binary format and printing to text format.

Makes sense, we do want that coverage. But adding a wast file in the same directory should get the same test coverage (it will also emit .fromBinary outputs etc. - it will write to binary, read, then print that). And text tests are generally easier to handle.

We do have some actual .wasm files in the test suite, but mainly to handle code that can't be directly represented in the text format for some reason.

kripken avatar Aug 17 '22 19:08 kripken