Skip to content
This repository was archived by the owner on Nov 3, 2021. It is now read-only.

Coordinate encoding with reftypes proposal to take into account multiple tables #36

Closed
lars-t-hansen opened this issue Oct 31, 2018 · 7 comments

Comments

@lars-t-hansen
Copy link
Contributor

See here. It would be beneficial if eg table.copy and table.init would choose encodings of flags and table indices that are compatible with those chosen by table.grow, table.fill, table.size, table.get, and table.set, introduced in that proposal. (As well as with call_indirect, possibly, although that may be a stretch.)

@lukewagner
Copy link
Member

Related, but not explicitly stated above: the proposal has table.copy/memory.copy taking a single memory/table index, when I think we should allow cross-memory/table copies and thus give these operations two indices each.

Another subtler point is that it would be good to note that the memory/table indices are validated to be in-bounds. Pre-multi-memory/table this implies that it's a validation error to use a bulk memory/table op when there is no memory/table definition.

@lars-t-hansen
Copy link
Contributor Author

We should probably further clarify that these placeholder zeroes are encoded as varuint32. The table operators proposed in the reftypes proposal certainly require that encoding at present, see eg the decoding algorithm for table.get in that repo. @binji, wdyt? Are we still holding out for the possibility of a "flags" encoding here? Personally I think it's time to make a call on this and state that these fields are straight table/memory indices.

@binji
Copy link
Member

binji commented Jan 25, 2019

No, flags is gone at this point I think. The only reason they're bytes is that's how other reserved bytes work, and it's safe to extend this representation to varuint32 later.

Maybe the right thing to do here is to add a note specifying how this proposal changes with the reftype proposal enabled -- since we still can assume that it can be shipped independently.

Then again, maybe it's silly to specify an encoding (i.e. reserved bytes) that we know will change immediately.

@lars-t-hansen
Copy link
Contributor Author

My vote is to go for varuint32 right now for the placeholder memory/table indices since it is exceedingly likely that that encoding will be the final outcome; I also expect that before bulk memory goes to stage 4 we will have reftypes so far along that we'll know if something radical has happened to change that proposal that would spill over to this one (both SpiderMonkey and v8 are making good progress on implementing reftypes, IIUC). @rossberg, @titzer, @lukewagner any opinions?

@rossberg
Copy link
Member

I'm fine either way. Byte vs u32 seems like a 5 minute change in spec or implementations. A minor argument for byte might be that it's probably less work to write negative tests for -- in case we want to spend the time doing that in the first place.

@binji
Copy link
Member

binji commented Feb 1, 2019

Having done some of the wabt/v8/spec interpreter work here, I've found it simpler to reuse the code that reads a required 0 byte for these instructions. The reference types proposal adds multiple tables, so it's the first place that adds code to read a u32 index -- switching them over to use that code then seems simpler to me, personally. Since we don't have multiple memories (and probably won't for a long time), we can keep them the same as memory.grow and memory.size.

@lars-t-hansen
Copy link
Contributor Author

The de facto resolution here is a required byte 0 for the table and memory index.

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

No branches or pull requests

4 participants