Repository navigation
Implement table.init - #6827
Implement table.init#6827
Conversation
| droppedElementSegments.count(curr->segment)) { | ||
| trap("out of bounds segment access in table.init"); | ||
| } | ||
| if ((uint64_t)offsetVal + sizeVal > segment->data.size()) { |
There was a problem hiding this comment.
What role does this uint64_t cast play?
There was a problem hiding this comment.
We use this pattern to avoid a 32-bit overflow.
There was a problem hiding this comment.
But Addresses are 64-bit already, right?
There was a problem hiding this comment.
Ah, fair point. Then this can be changed in the MemoryInit code I copied this from. I'll do that.
| trap("out of bounds table access in table.init"); | ||
| } | ||
| for (size_t i = 0; i < sizeVal; ++i) { | ||
| auto value = self()->visit(segment->data[offsetVal + i]).getSingleValue(); |
There was a problem hiding this comment.
This is not correct, but I think it's the same as a bug we had with the instantiation logic before this change. Maybe it's a good time to fix it.
The problem is that a segment is a sequence of values, not instructions. If you do a table.init of a segment containing a bunch of struct.new instructions, you should get the same struct references every time, not fresh struct references.
There was a problem hiding this comment.
Good point, yeah, this is a bug that existed before and this refactoring just moves around a bit.
I'm actually not sure how best to fix it - seems like it requires some new data structure, at least - so I'll add a FIXME here for later.
| TableInit curr; | ||
| CHECK_ERR(visitTableInit(&curr)); |
There was a problem hiding this comment.
This is going to need the table to be set on curr so it can figure out whether to read an i32 or an i64. Since we're not using child-typer in the validator yet, the only way to test that this is correct is by seeing how the instruction is parsed when one of its subsequent children is unreachable.
There was a problem hiding this comment.
I don't follow. There is no implementation of visitTableInit - it's empty, then, from the parent of IRBuilder, which is UnifiedExpressionVisitor. That calls visitExpression, but there is no special code from TableInit there either. Am I mixed up..?
If this is some table64 complexity, then perhaps it is simpler if I make this PR only support table.init on wasm32?
There was a problem hiding this comment.
visitTableInit ends up in visitExpression, which calls ConstraintCollector::visit. ConstraintCollector inherits from ChildTyper, which should implement a specific visitTableInit that will need to look up the table to figure out the index type.
But yes, this is to properly support table64, so one option would be to punt that to later. I think it would be better to handle it now, though.
There was a problem hiding this comment.
Ah, thanks, it is indeed in that other class. Done.
| ;; CHECK-NEXT: ) | ||
| ;; CHECK-NEXT: ) | ||
| (func $test_table_init | ||
| (table.init $t64 $elem64 (i64.const 0) (i64.const 5) (i64.const 10)) |
There was a problem hiding this comment.
As mentioned in the comments on the validator, this should be invalid because only the first operand should be an i64.
| shouldBeEqualOrFirstIsUnreachable(curr->size->type, | ||
| destTable->indexType, | ||
| curr, | ||
| "table.copy size must be valid"); |
There was a problem hiding this comment.
This actually needs to be i32 if either of the input tables are 32-bit and i64 otherwise.
| shouldBeEqualOrFirstIsUnreachable(curr->offset->type, | ||
| table->indexType, | ||
| curr, | ||
| "table.init offset must be valid"); | ||
| shouldBeEqualOrFirstIsUnreachable( | ||
| curr->size->type, Type(Type::i32), curr, "table.copy size must be i32"); | ||
| curr->size->type, table->indexType, curr, "table.init size must be valid"); |
There was a problem hiding this comment.
Only the destination is an i64 if the table is 64-bit. The offset and size are both still i32s.
| } | ||
|
|
||
| void visitTableInit(TableInit* curr) { | ||
| note(&curr->dest, Type::i32); |
There was a problem hiding this comment.
This needs to be i64 if the referenced table is 64-bit.
|
Thanks for the helpful comments, all the validation and other issues should now be addressed. |
2 participants
Also use TableInit in the interpreter to initialize module's table state, which will
now handle traps properly, fixing #6431
The wasm-ctor-eval addition of tableSize is necessary here, as now tableSize
is called internally from the interpreter as part of using TableInit.