Skip to content

Conversation

@rossberg
Copy link
Member

@rossberg rossberg commented Oct 19, 2016

The MVP allows only one static memory or table, and respective operators do not index a table explicitly. Loads and stores already have a flag immediate that could be instrumented in a future extension to allow them to access either multiple memories or even first-class memories.

This PR reserves a flag byte for the other 3 remaining instructions on memories and tables: current_memory, grow_memory and call_indirect. That way, a future extension to multiple or first class tables/memories will not require duplicating any opcode.

@rossberg rossberg changed the title Future-proof *_memory operators for multiple memories Future-proof memory instructions Oct 19, 2016
@rossberg rossberg added this to the MVP milestone Oct 19, 2016
@titzer
Copy link

titzer commented Oct 19, 2016

It's not clear how we will extend the load/store instructions in the future. E.g. we could extend the flags field (which currently only contains the offset), which means no new instructions. Or, we could introduce prefixes. If we introduce prefixes, then the prefixes could as well apply to these instructions.

So I'd rather we not add this, since if we introduce new instructions, or add prefixes, these immediates will be unnecessary.

@lukewagner
Copy link
Member

Actually, I thought of one not-totally-subjective reason that puts me in favor of this proposal:

We've talked about 3 ways to indicate which Memory/Table you want a given op to access:

  1. the default (the only option in the MVP)
  2. an imported or internally-defined table identified by immediate index (post-MVP, when multiple memories/tables are allowed)
  3. a dynamic table passed by first-class reference on the operand stack (post-MVP, with the GC feature)

Initially, I thought that 2 was of mild value, since 3 is more general and so I've generally assumed we wouldn't add 2. But I've remembered one useful property of 2: it categorically avoids using the GC feature which I expect will end up carrying some cost in implementations, even if it is only very lightly used. And while multiple memories is of questionable value outside of GC contexts, I know compiled C/C++ code can make use of multiple tables, once given homogeneous table types, to avoid the signature check on call_indirect. Also there are CFI applications.

So if we were able to express 1-3 just using the flags fields, then we save 50 ops which, even if "ops are cheap", is probably worth something. E.g., it might make the difference between all non-SIMD ops fitting in the 1-byte encoding. So from that, this PR follows. Sure we could do something custom for current_memory/grow_memory, but symmetry.

@titzer
Copy link

titzer commented Oct 19, 2016

On the other hand, this would be the only change to 0xD other than a simple renumbering, which makes WABT's 0xC -> 0xD translation a tad harder.

@lukewagner
Copy link
Member

That's true, but it should still be highly mechanical.

@lukewagner
Copy link
Member

Actually, based on the reasoning that "any op that uses the default (memory|table) should have a flags immediate so that we can add non-default options in the future", it seems that call_indirect should also get a flags.

@rossberg
Copy link
Member Author

@lukewagner, added.

@lukewagner
Copy link
Member

Thanks, lgtm

@titzer
Copy link

titzer commented Oct 21, 2016

I see the use case for multiple static tables in the future, and although I'm more skeptical about multiple static memories, I think it's fine to land this change as is, so LGTM

@rossberg
Copy link
Member Author

Landing with LGTMs above.

@rossberg rossberg merged commit 4f77107 into binary_0xd Oct 21, 2016
@rossberg rossberg deleted the mem-index branch October 21, 2016 09:12
lukewagner added a commit that referenced this pull request Oct 26, 2016
* Reorganise opcode space (#826)

* Future-proof type opcodes (#823)

* Future-proof memory instructions (#824)

* Fix order of copysign (#828)

* Global types can be mutable, just not when imported (#830)

* Naming nit: rename `flags` to `reserved` (#832)

* Bump 0xd's version to 0xd

* Make it hex
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants