[webkit-reviews] review granted: [Bug 219427] [WASM-References] Add support for table.copy : [Attachment 415276] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Dec 4 14:28:12 PST 2020
Yusuke Suzuki <ysuzuki at apple.com> has granted Dmitry <dbezhetskov at igalia.com>'s
request for review:
Bug 219427: [WASM-References] Add support for table.copy
https://bugs.webkit.org/show_bug.cgi?id=219427
Attachment 415276: Patch
https://bugs.webkit.org/attachment.cgi?id=415276&action=review
--- Comment #7 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 415276
--> https://bugs.webkit.org/attachment.cgi?id=415276
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=415276&action=review
r=me with nits.
> Source/JavaScriptCore/bytecode/BytecodeList.rb:1618
> + dstTableIdx: unsigned,
> + srcTableIdx: unsigned,
Let's use Index instead of Idx since the other bytecodes use Index / index.
> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1180
> +auto AirIRGenerator::addTableCopy(unsigned dstTableIdx, unsigned
srcTableIdx, ExpressionType dstOffset, ExpressionType srcOffset, ExpressionType
length) -> PartialResult
> +{
> + ASSERT(dstOffset.tmp());
> + ASSERT(dstOffset.type() == Type::I32);
> +
> + ASSERT(srcOffset.tmp());
> + ASSERT(srcOffset.type() == Type::I32);
> +
> + ASSERT(length.tmp());
> + ASSERT(length.type() == Type::I32);
> +
> + auto result = tmpForType(Type::I32);
> + emitCCall(
> + &operationWasmTableCopy, result, instanceValue(),
> + addConstant(Type::I32, dstTableIdx),
> + addConstant(Type::I32, srcTableIdx),
Let's use Index instead of Idx.
> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:792
>
> +auto B3IRGenerator::addTableCopy(unsigned dstTableIdx, unsigned srcTableIdx,
ExpressionType dstOffset, ExpressionType srcOffset, ExpressionType length) ->
PartialResult
> +{
> + auto result = m_currentBlock->appendNew<CCallValue>(
> + m_proc, toB3Type(I32), origin(),
> + m_currentBlock->appendNew<ConstPtrValue>(m_proc, origin(),
tagCFunction<OperationPtrTag>(operationWasmTableCopy)),
> + instanceValue(),
> + m_currentBlock->appendNew<Const32Value>(m_proc, origin(),
dstTableIdx),
> + m_currentBlock->appendNew<Const32Value>(m_proc, origin(),
srcTableIdx),
> + dstOffset, srcOffset, length);
Let's use Index instead of Idx.
> Source/JavaScriptCore/wasm/WasmFunctionParser.h:683
> + unsigned dstTableIdx;
> + WASM_PARSER_FAIL_IF(!parseVarUInt32(dstTableIdx), "can't parse
destination table index");
> + WASM_VALIDATOR_FAIL_IF(dstTableIdx >= m_info.tableCount(),
"table index ", dstTableIdx, " is invalid, limit is ", m_info.tableCount());
> +
> + unsigned srcTableIdx;
> + WASM_PARSER_FAIL_IF(!parseVarUInt32(srcTableIdx), "can't parse
source table index");
> + WASM_VALIDATOR_FAIL_IF(srcTableIdx >= m_info.tableCount(),
"table index ", srcTableIdx, " is invalid, limit is ", m_info.tableCount());
> +
> + const auto srcType = m_info.table(srcTableIdx).wasmType();
> + const auto dstType = m_info.table(dstTableIdx).wasmType();
> + WASM_VALIDATOR_FAIL_IF(srcType != dstType, "type mismatch at
table.copy. got ", srcType, " and ", dstType);
> +
> + TypedExpression dstOffset;
> + TypedExpression srcOffset;
> + TypedExpression length;
> + WASM_TRY_POP_EXPRESSION_STACK_INTO(length, "table.copy");
> + WASM_TRY_POP_EXPRESSION_STACK_INTO(srcOffset, "table.copy");
> + WASM_TRY_POP_EXPRESSION_STACK_INTO(dstOffset, "table.copy");
> +
> + WASM_VALIDATOR_FAIL_IF(I32 != dstOffset.type(), "table.copy
dst_offset to type ", dstOffset.type(), " expected ", I32);
> + WASM_VALIDATOR_FAIL_IF(I32 != srcOffset.type(), "table.copy
src_offset to type ", srcOffset.type(), " expected ", I32);
> + WASM_VALIDATOR_FAIL_IF(I32 != length.type(), "table.copy length
to type ", length.type(), " expected ", I32);
> +
> + WASM_TRY_ADD_TO_CONTEXT(addTableCopy(dstTableIdx, srcTableIdx,
dstOffset, srcOffset, length));
Ditto.
> Source/JavaScriptCore/wasm/WasmInstance.cpp:123
> +void Instance::tableCopy(uint32_t dstOffset, uint32_t srcOffset, uint32_t
len, uint32_t dstTableIdx, uint32_t srcTableIdx)
Let's use `length` instead of `len`.
> Source/JavaScriptCore/wasm/WasmInstance.cpp:132
> + auto forTableElement = [&] (auto fn) {
Don't put space between `[]` and `(...)` in new code.
And `forEachTableElement` would be better name for this function.
> Source/JavaScriptCore/wasm/WasmInstance.cpp:134
> + for (uint32_t idx = len; idx--;)
Let's use index.
> Source/JavaScriptCore/wasm/WasmInstance.cpp:139
> + for (uint32_t idx = 0; idx < len; ++idx)
Ditto.
> Source/JavaScriptCore/wasm/WasmInstance.cpp:145
> + forTableElement([] (Table* dstTable, Table* srcTable, uint32_t
dstIdx, uint32_t srcIdx) {
Don't put space between `[]` and `(Table* dstTable, Table* srcTable, uint32_t
dstIdx, uint32_t srcIdx)` in new code.
> Source/JavaScriptCore/wasm/WasmInstance.cpp:151
> + forTableElement([] (Table* dstTable, Table* srcTable, uint32_t dstIdx,
uint32_t srcIdx) {
Ditto.
> Source/JavaScriptCore/wasm/WasmInstance.h:80
> + void tableCopy(uint32_t dstOffset, uint32_t srcOffset, uint32_t len,
uint32_t dstTableIdx, uint32_t srcTableIdx);
Ditto for Idx and len.
> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:1149
> +auto LLIntGenerator::addTableCopy(unsigned dstTableIdx, unsigned
srcTableIdx, ExpressionType dstOffset, ExpressionType srcOffset, ExpressionType
length) -> PartialResult
> +{
> + WasmTableCopy::emit(this, dstOffset, srcOffset, length, dstTableIdx,
srcTableIdx);
> + return { };
> +}
Ditto.
> Source/JavaScriptCore/wasm/WasmModuleInformation.h:70
> + const TableInformation& table(unsigned idx) const { return tables[idx];
}
Ditto.
> Source/JavaScriptCore/wasm/WasmOperations.cpp:759
> +JSC_DEFINE_JIT_OPERATION(operationWasmTableCopy, bool, (Instance* instance,
unsigned dstTableIdx, unsigned srcTableIdx, int32_t dstOffset, int32_t
srcOffset, int32_t length))
> +{
> + ASSERT(dstTableIdx <
instance->module().moduleInformation().tableCount());
> + ASSERT(srcTableIdx <
instance->module().moduleInformation().tableCount());
> + const Table* dstTable = instance->table(dstTableIdx);
> + const Table* srcTable = instance->table(srcTableIdx);
> + ASSERT(dstTable->type() == srcTable->type());
> +
> + if ((srcOffset < 0) || (dstOffset < 0) || (length < 0))
> + return false;
> +
> + Checked<uint32_t, RecordOverflow> lastDstElementIdxChecked =
static_cast<uint32_t>(dstOffset);
> + lastDstElementIdxChecked += static_cast<uint32_t>(length);
> +
> + uint32_t lastDstElementIdx;
> + if (lastDstElementIdxChecked.safeGet(lastDstElementIdx) ==
CheckedState::DidOverflow)
> + return false;
> +
> + if (lastDstElementIdx > dstTable->length())
> + return false;
> +
> + Checked<uint32_t, RecordOverflow> lastSrcElementIdxChecked =
static_cast<uint32_t>(srcOffset);
> + lastSrcElementIdxChecked += static_cast<uint32_t>(length);
> +
> + uint32_t lastSrcElementIdx;
> + if (lastSrcElementIdxChecked.safeGet(lastSrcElementIdx) ==
CheckedState::DidOverflow)
> + return false;
> +
> + if (lastSrcElementIdx > srcTable->length())
> + return false;
> +
> + instance->tableCopy(dstOffset, srcOffset, length, dstTableIdx,
srcTableIdx);
> + return true;
> +}
Ditto.
> Source/JavaScriptCore/wasm/WasmTable.cpp:139
> +void Table::copy(const Table* srcTable, uint32_t dstIdx, uint32_t srcIdx)
> +{
> + RELEASE_ASSERT(isExternrefTable());
> + RELEASE_ASSERT(srcTable->isExternrefTable());
> +
> + set(dstIdx, srcTable->get(srcIdx));
> +}
Ditto.
> Source/JavaScriptCore/wasm/WasmTable.cpp:226
> +void FuncRefTable::copyFunction(const FuncRefTable* srcTable, uint32_t
dstIdx, uint32_t srcIdx)
> +{
> + if (srcTable->get(srcIdx).isNull()) {
> + clear(dstIdx);
> + return;
> + }
> +
> + setFunction(dstIdx, jsCast<JSObject*>(srcTable->get(srcIdx)),
srcTable->function(srcIdx), srcTable->instance(srcIdx));
> +}
Ditto.
> Source/JavaScriptCore/wasm/WasmTable.h:79
> + void copy(const Table* srcTable, uint32_t dstIdx, uint32_t srcIdx);
Ditto.
> Source/JavaScriptCore/wasm/WasmTable.h:105
> + void copyFunction(const FuncRefTable* srcTable, uint32_t dstIdx,
uint32_t srcIdx);
Ditto.
More information about the webkit-reviews
mailing list