Coordinate with bulk table ops / clarify encoding of the table index about reference-types

[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.

[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.

[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.)

[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.

[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.