Coordinate with bulk table ops / clarify encoding of the table index

[lars-t-hansen]

This proposal introduces multiple tables, a new thing. This means that some existing operations must be expanded (call_indirect needs to carry an optional table index, although it has space for this) and that some proposed operations must take the multi-table case into account (table.copy, table.init). We should be sure to harmonize with the bulk memory proposal now.

The bulk table operations have a (misnamed) memory varu32 operand that must currently be zero and can be repurposed as a flags field /or/ as a table index field, see here, depending on how we like it. For table.copy there can be two table indices, so a flags field is probably more or less inevitable for that instruction.

I propose that we uniformly use a varu32 flags field for all the table operations: table.get, table.set, table.grow, table.fill, table.size, table.copy, and table.init. (table.drop is arguably misnamed but that's a discussion for elsewhere) and that this field is either zero, indicating default operands (table zero for every instruction except table.copy; src=0 and dest=0 for table.copy), or a bit indicating that a table index follows after the flag word, or, for table.copy, that two indices follow, for dest and source.

Normally, having a flags field will add zero overhead when using table zero, and one byte of overhead when using other tables.

For prototype code I'm writing for Firefox I've gone with the flag value 0x04 to signify the presence of table indices, to fit in with the other flags that we currently use for memories and tables (0x00=Default, 0x01=HasMaximum, 0x02=IsShared).

@rossberg, opinions? We don't have to use a flags field everywhere but we probably have to use it for the bulk table operations, which strongly suggests using one for table.grow and table.fill and table.size; table.get and table.set and call_indirect might reasonably be considered a separate class of instructions. The cost of using a flags field there seems slight. We can choose not to do it, but then any future extensions that could have used a flag will require new instructions or out-of-band encodings in the table index.

[rossberg]

I wasn't aware that the bulk memory instructions do not already have mem/table indices. That seems like a mistake from my perspective. We made sure that all affected instructions like call_indirect, memory.*, and segment definitions anticipate the necessary index for multiple tables/memories, so we should keep going with that. The only outliers are load/store, where we have space in the memarg as an extension point.

Not a big fan of repurposing these existing index immediates as flag fields. What's the benefit? It would at best save a byte or two for rare opcodes like bulk instructions, which doesn't seem worth the irregularity.

Not sure I understand how such flag fields would relate to the flags in memory/table types. AFAICS, that is a completely disjoint part of the binary grammar.

[alexcrichton]

@lars-t-hansen would this perhaps be a good issue to bring up with the next CG meeting? Some questions we could try to work through are:

• Should we stick with 1-byte opcodes for table.{get,set} and try to allocate similar ones for table.{grow,size}? (similarly, should table.{init,drop,copy} follow suit or stay with a prefix?)
• Should table instructions have a byte for flags? (this issue)
• Having two indices for memory on memory.copy and two indices for tables on table.copy. (not directly related to this proposal, but maybe a good time to bring it up anyway)

[binji]

The bulk table operations have a (misnamed) memory varu32 operand...

Fixed here.

For table.copy there can be two table indices, so a flags field is probably more or less inevitable for that instruction.

@AndrewScheidecker brought up this issue here: WebAssembly/bulk-memory-operations#29

I wasn't aware that the bulk memory instructions do not already have mem/table indices.

They have a required zero byte, similar to memory.{grow,size} and call_indirect.

Not a big fan of repurposing these existing index immediates as flag fields. What's the benefit?

AIUI, to allow for further extending the instruction's behavior.

[binji]

would this perhaps be a good issue to bring up with the next CG meeting?

Yes, I guess we can extend the discussion that I already added for renaming memory.drop and table.drop.

[lars-t-hansen]

Not sure I understand how such flag fields would relate to the flags in memory / table types. AFAICS, that is a completely disjoint part of the binary grammar.

Only that it's a related domain and that if we wish we can choose from the same set instead of introducing another set. Doesn't really matter to me. Personally I'm not sure that we need the flags at all, just having the indices seems sufficient. But we have discussed in the past whether the bulk /memory/ operations need memargs, without reaching a firm conclusion about that.

(Re the single vs the double operand to memory.copy and table.copy, if we don't have a flag byte we surely must have two operands, so sticking with a single byte commits us to using a flag in that case. It will be good to get this settled.)

[binji]

Well, it seems we've mostly aligned on having two indices for the *.copy instructions, so I've created a PR for that.

I apologize, we were meant to discuss this issue at the last two CG meetings, but I had forgotten to add it to the agenda. I can do so for the next meeting in two weeks, but I think we could also continue to discuss here.

It seems that no one is convinced that we need a flags field, and that two indices should be sufficient. Allowing future support of memargs is a possibility, but if we decide that's useful, we can always add new instructions. I don't think it's even that much of a wart to do so. :-)

[lars-t-hansen]

Since there doesn't seem to be much discussion about this any longer, I'm going to assume that all these immediates are plain indices (memory index / table index), not flags.

Summarizing table operations from both the bulk memory proposal and the reftypes proposal:

(table.init element-segment-index table-index)
(table.drop data-segment-index)
(table.copy src-table-index dest-target-index)
(table.get table-index)
(table.set table-index)
(table.grow table-index)
(table.size table-index)
(table.fill table-index)
(table.drop element-segment-index)

Here, table.get and table.set are from reftypes, and table.fill, table.size, and table.grow are proposed for inclusion in this proposal though (so far as I can tell) not currently defined here.

Summarizing memory operations from the bulk memory proposal:

(memory.init data-segment-index memory-index=0x00)
(memory.drop data-segment-index)
(memory.copy src-memory-index=0x00 dest-memory-index=0x00)
(memory.fill memory-index=0x00)

It would be good to move this issue along. It would be silly/unfortunate for, say, the bulk memory ops to suddenly decode some immediates as flags if the instructions from the reftypes proposal have shipped and can't do that; so in some sense, bulk memory is blocking reftypes here.

@binji, can we get this on the agenda for Tuesday? There's not a doc there yet or I'd file the PR myself.

[EDIT: I was confused about which proposal this ticket was located in. Clarified.]

[lars-t-hansen]

On that note, the PR that added the second placeholder byte to memory.copy and table.copy also updated Overview.txt so that the operand order (as in my list above) is source followed by destination. But the order of the non-immediate operands is destination followed by source. It would perhaps be better if there was agreement here.

[rossberg]

Considering that the reference types proposal seems to be moving fast lately in terms of implementation, should we perhaps consider moving all the table bulk instructions over to that proposal consistently? That would simplify the dependencies and avoid the coordination issue altogether.

[lars-t-hansen]

Considering that the reference types proposal seems to be moving fast lately in terms of implementation, should we perhaps consider moving all the table bulk instructions over to that proposal consistently? That would simplify the dependencies and avoid the coordination issue altogether.

That's fine with me in principle, though it leaves us with "having" to coordinate memory bulk ops and table bulk ops - so I'm not sure it gains us very much. (What I would really like to see is moving the bulk ops proposal forward. We've had an implementation for longer than we've had the reftypes implementation, but since the encodings have changed I have to update it.)

[binji]

@lars-t-hansen sorry about that, added here.

What I would really like to see is moving the bulk ops proposal forward.

Yes, we (on v8) would like to see this too. Our implementation is almost complete, so I think we need updates to the spec text and some tests to be able advance to phase 3.

[titzer]

We're almost finished with the bulk memory ops implementation in V8. The table ops were the last to be done--just table.init remains. I'm fine either keeping or moving table ops to reference types, but have a slight preference for avoiding any logistical issues from scrambling things again. As for the V8 implementation perspective, it is just a question of which flag guards which functionality.

[lars-t-hansen]

I too would prefer not to split, it was just offered as a way out in case we could not completely agree on the table ops. (We're also almost finished with bulk memory + bulk tables, mostly what's left is updating the encodings and fixing some semantic fine points.)

[binji]

OK, let's plan not to split for now.

[gahaas]

I just realized that the encoding of the element section in the reference-types proposal and the bulk-memory-operations proposal is conflicting. In the bulk-memory-operations proposal, table indices > 0 are prefixed with 0x2, in the reference-types proposal they are not. Are there already plans to synchronize that?

[binji]

Yes, the bulk memory proposal did it this way so we could use designate passive segments (using flag byte = 0x1). So we need to change the reference types proposal to follow the same model.

[rossberg]

Landed #35, which adjusted encoding of element segment to match bulk ops proposal and added missing table.fill/grow/size operators. I think we can close this now.