Skip to content

Multiple tables support from the reference-types proposal #3512

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

Closed
martianboy opened this issue Jan 23, 2021 · 3 comments · Fixed by #3517
Closed

Multiple tables support from the reference-types proposal #3512

martianboy opened this issue Jan 23, 2021 · 3 comments · Fixed by #3517

Comments

@martianboy
Copy link
Contributor

martianboy commented Jan 23, 2021

Hi, recently I've been curious about the reference-types proposal and I was trying to play around with it in various tools when I found out that wasm-opt doesn't like it when I give it a wasm file with multiple tables. So for the last 2-3 days I started digging through Binaryen's codes as well as some of the communications happening on this Github repo, trying to understand the situation.

So far, I found PR #2451 which added support for reference-types, but left multiple tables support and table instructions out. I also checked PR #3084 quickly, that brought the existing implementation up to date with recent changes from the proposal.

But nowhere can I find any mention of the missing support for multiple tables, which makes me feel like I am missing something: Has it been a non-goal for Binaryen to support this, or somehow the cost hasn't been worth the effort to fully implement the remaining parts, or it's just been low on priority. This is just me being curious.

I also went through the codebase, played around with the idea of replacing the single table object in Binaryen's Wasm module IR, with a std::vector<std::unique_ptr<Table>> just like the other module elements. With some help I will be able to publish it as a PR, potentially preparing the stage for full implementation of the proposal in the future.

So I just wanted to check whether I should continue this work, or there are important notes that I'm missing. I'd also appreciate any insight or directions.

@aheejin
Copy link
Member

aheejin commented Jan 25, 2021

As you noticed, Binaryen does not yet support multiple tables. We only have reference types and subtype relationship for now. We didn't implement it at the time just because it was not a high priority for us, because our LLVM toolchain did not generate multiple tables. @sbc100, do we support multiple tables in LLVM now?

Either way, you can submit PRs for its support and we would appreciate it.

@martianboy
Copy link
Contributor Author

martianboy commented Jan 25, 2021

Thanks for the reply. I come from a RustWasm and wasm-bindgen background which does have an option for reference-types to generate multiple tables.

I am almost finished with the first stage of my changes, just trying to make sure the test suite passes without any substantial change in the functionality. I will submit a PR once it's done, and I will appreciate reviews, comments, etc. :)

@sbc100
Copy link
Member

sbc100 commented Jan 25, 2021

The multi-table support in llvm/lld is being worked on right now by @wingo and should land any day now. So it does seem like the right time to add multi-table support for binaryen. Any PRs to add such support I think would be very welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants