Skip to content
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

Add support for the reference types proposal #938

Merged
merged 2 commits into from Feb 14, 2019

Conversation

alexcrichton
Copy link
Contributor

This commit adds support for the reference types proposal to wabt.
Namely it adds new opcodes like table.{get,set,grow} as well as adds a
new anyref type. These are plumbed throughout for various operations
in relatively simple fashions, no support was added for a subtyping
relationship between anyref and anyfunc just yet.

This also raises the restriction that multiple tables are disallowed,
allowing multiple tables to exist when --enable-reference-types is
passed.

@alexcrichton
Copy link
Contributor Author

One final thing I know of that would want to be sorted out before merging is the binary representation of the opcodes. Currently the reference types overview doesn't go into detail about the binary encodings and the table instructions specification doesn't currently mention table.grow (yet!). The experimental Gecko implementation, however, chooses a different encoding for table.{get,set} as well as picks an encoding for table.grow.

I think it should be easy to sort this out here whichever way we want it to be, I just want to make sure that this isn't merged before that's settled! I also don't mind sending a PR to the reference-types proposal with an update of encodings too!

@alexcrichton
Copy link
Contributor Author

Oh I should also mentioned that this goes ahead and uses the renamings proposed of table.{get,set,grow} rather than get_table and set_table and such

@alexcrichton
Copy link
Contributor Author

Oh hm I'm also not entirely sure where travis is going awry with the asmjs build (I think?). Is there perhaps a file I need to regenerate somewhere as well?

@binji
Copy link
Member

binji commented Oct 30, 2018

Oh hm I'm also not entirely sure where travis is going awry with the asmjs build (I think?).

It's an unrelated error, I believe. I filed a bug here: #939

Copy link
Member

@binji binji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, mostly just nits.

src/binary-reader-ir.cc Outdated Show resolved Hide resolved
src/binary-reader-ir.cc Outdated Show resolved Hide resolved
src/binary-reader-nop.h Outdated Show resolved Hide resolved
src/binary-reader.h Outdated Show resolved Hide resolved
test/typecheck/bad-reference-types-no-table.txt Outdated Show resolved Hide resolved
@alexcrichton alexcrichton force-pushed the reference-types branch 2 times, most recently from 10790fd to 0c41e09 Compare October 30, 2018 22:04
@lars-t-hansen
Copy link

I think there are many open questions around encoding. I have filed a bug here: WebAssembly/reference-types#18 to get some eyes on this. Basically, the bulk memory ops will require work for multi-table support and we could usefully coordinate the encodings between the two proposals. I'll file a similar bug for bulk memory.

@alexcrichton
Copy link
Contributor Author

Ok I've rebased over the changes that have happened inbetween since this started, and I was curious if perhaps since the binary encoding is still in-the-air-but-implemented-in-a-few-places it might be good to get this in as a starting point and the binary format can be tweaked as the spec evolves?

@binji
Copy link
Member

binji commented Feb 13, 2019

Yes, I think we probably should (same as with other in-progress proposals). See further discussion about the instruction encoding here: WebAssembly/reference-types#29

Copy link
Member

@binji binji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor things. Since this PR has been waiting for a while, we can land this and I can fix after, or you can fix if you like, whichever you prefer.

src/binary-writer.cc Outdated Show resolved Hide resolved
src/wat-writer.cc Outdated Show resolved Hide resolved
src/wat-writer.cc Outdated Show resolved Hide resolved
src/wat-writer.cc Outdated Show resolved Hide resolved
src/wat-writer.cc Outdated Show resolved Hide resolved
src/wast-parser.cc Outdated Show resolved Hide resolved
@alexcrichton alexcrichton force-pushed the reference-types branch 2 times, most recently from ef1eacc to 472ed8a Compare February 14, 2019 15:23
This commit adds support for the reference types proposal to wabt.
Namely it adds new opcodes like `table.{get,set,grow}` as well as adds a
new `anyref` type. These are plumbed throughout for various operations
in relatively simple fashions, no support was added for a subtyping
relationship between `anyref` and `anyfunc` just yet.

This also raises the restriction that multiple tables are disallowed,
allowing multiple tables to exist when `--enable-reference-types` is
passed.
@alexcrichton
Copy link
Contributor Author

Ok thanks for the review! I think I've covered all those points and increased the test coverage some more as well (embarassing bugs!)

I've also gone ahead and updated all the encodings to match the latest proposal in WebAssembly/reference-types#29 which looks to be the way forward

Plumb support throughout for the `call_indirect` instruction (and
`return_call_indirect`) to work with multi-table modules according to
the reference types proposal.
Copy link
Member

@binji binji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

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 this pull request may close these issues.

None yet

3 participants