[WASM-References] Add table.init
Created attachment 414892 [details] Patch
This patch modifies one of the wasm.json files. Please ensure that any changes in one have been mirrored to the other. You can find the wasm.json files at "Source/JavaScriptCore/wasm/wasm.json" and "JSTests/wasm/wasm.json".
Created attachment 414915 [details] Patch
Comment on attachment 414915 [details] Patch This patch seems to have two patches combined together. Can you upload table.init on its own?
(In reply to Saam Barati from comment #4) > Comment on attachment 414915 [details] > Patch > > This patch seems to have two patches combined together. Can you upload > table.init on its own? Ah, sorry for this, still don't understand how to push two dependent patches. BTW, I want to land table.copy with spec-tests first and then land table.init because table.init ref-types spec depends on table.copy (https://github.com/WebAssembly/reference-types/blob/master/test/core/table_init.wast#L156).
<rdar://problem/71931757>
Created attachment 415626 [details] Patch
Comment on attachment 415626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415626&action=review The direction looks good, but I found several issues. > Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1163 > + auto result = tmpForType(Type::I32); > + emitCCall( > + &operationWasmElemDrop, result, instanceValue(), > + addConstant(Type::I32, elementIndex)); > + > + emitCheck([&] { > + return Inst(BranchTest32, nullptr, Arg::resCond(MacroAssembler::Zero), result, result); > + }, [=] (CCallHelpers& jit, const B3::StackmapGenerationParams&) { > + this->emitThrowException(jit, ExceptionType::OutOfBoundsTableAccess); > + }); > + > + return { }; Do we need exception check here? Looks like elem_drop instruction never returns false. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:787 > + auto result = m_currentBlock->appendNew<CCallValue>( > + m_proc, toB3Type(I32), origin(), > + m_currentBlock->appendNew<ConstPtrValue>(m_proc, origin(), tagCFunction<OperationPtrTag>(operationWasmElemDrop)), > + instanceValue(), > + m_currentBlock->appendNew<Const32Value>(m_proc, origin(), elementIndex)); > + > + { > + CheckValue* check = m_currentBlock->appendNew<CheckValue>(m_proc, Check, origin(), > + m_currentBlock->appendNew<Value>(m_proc, Equal, origin(), result, m_currentBlock->appendNew<Const32Value>(m_proc, origin(), 0))); > + > + check->setGenerator([=] (CCallHelpers& jit, const B3::StackmapGenerationParams&) { > + this->emitExceptionCheck(jit, ExceptionType::OutOfBoundsTableAccess); > + }); > + } > + Ditto. > Source/JavaScriptCore/wasm/WasmInstance.cpp:168 > +const Element* Instance::elem(unsigned i) const Let's name it `elementAt`. > Source/JavaScriptCore/wasm/wasm.json:72 > + "table.init": { "category": "exttable", "value": 252, "return": ["i32"], "parameter": ["i32", "i32", "i32"], "immediate": [{"name": "element_index", "type": "varuint32"}, {"name": "table_index","type": "varuint32"}],"description": "initialize table from a given element", "extendedOp": 12 }, Does `table.init` return "i32" value? Can you point at the spec what value should be returned? > Source/JavaScriptCore/wasm/wasm.json:73 > + "elem.drop": { "category": "exttable", "value": 252, "return": ["i32"], "parameter": [], "immediate": [{"name": "element_index", "type": "varuint32"}], "description": "prevents further use of a passive element segment", "extendedOp": 13 }, Ditto for elem.drop. And can you check the other table instructions too?
Comment on attachment 415626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415626&action=review > Source/JavaScriptCore/wasm/WasmOperations.cpp:724 > +JSC_DEFINE_JIT_OPERATION(operationWasmElemDrop, bool, (Instance* instance, unsigned elementIndex)) Let's make return type "void" since it always returns "true".
Comment on attachment 415626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415626&action=review >> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:1163 >> + return { }; > > Do we need exception check here? Looks like elem_drop instruction never returns false. Ah, thanks, fixed! >> Source/JavaScriptCore/wasm/WasmInstance.cpp:168 >> +const Element* Instance::elem(unsigned i) const > > Let's name it `elementAt`. renamed >> Source/JavaScriptCore/wasm/WasmOperations.cpp:724 >> +JSC_DEFINE_JIT_OPERATION(operationWasmElemDrop, bool, (Instance* instance, unsigned elementIndex)) > > Let's make return type "void" since it always returns "true". Done >> Source/JavaScriptCore/wasm/wasm.json:73 >> + "elem.drop": { "category": "exttable", "value": 252, "return": ["i32"], "parameter": [], "immediate": [{"name": "element_index", "type": "varuint32"}], "description": "prevents further use of a passive element segment", "extendedOp": 13 }, > > Ditto for elem.drop. And can you check the other table instructions too? Yeah, I noticed that too when I looked at table.fill, copied code and forgot about it :) Fixed table.copy, elem.drop and table.fill.
Created attachment 415733 [details] Patch
Created attachment 415747 [details] Patch
Comment on attachment 415747 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415747&action=review Nice. I'm putting r- since I found several bugs. > Source/JavaScriptCore/wasm/WasmFunctionParser.h:621 > + WASM_PARSER_FAIL_IF(!parseVarUInt32(elementIndex), "can't parse element index"); > + WASM_VALIDATOR_FAIL_IF(elementIndex >= m_info.elementCount(), "element index ", elementIndex, " is invalid, limit is ", m_info.elementCount()); > + > + unsigned tableIndex; > + WASM_PARSER_FAIL_IF(!parseVarUInt32(tableIndex), "can't parse element index"); > + WASM_VALIDATOR_FAIL_IF(tableIndex >= m_info.tableCount(), "table index ", tableIndex, " is invalid, limit is ", m_info.tableCount()); This parsing behavior is different from the other table ext operations. This means that we need to have special handling in `FunctionParser<Context>::parseUnreachableExpression()` side too. The purpose of `FunctionParser<Context>::parseUnreachableExpression()` is skip all wasm stack pushing/poping, but validates imm etc. for unreachable wasm opcodes. Can you add them? I think we need TableCopy one too. And, can you add tests for these cases? I think we can just emit these opcodes after return, and ensure we are stressing table opcodes in parseUnreachableExpression. > Source/JavaScriptCore/wasm/WasmInstance.cpp:73 > + for (unsigned elementIdx = 0; elementIdx < m_passiveElements.size(); ++elementIdx) { Rename idx to index. > Source/JavaScriptCore/wasm/WasmInstance.cpp:183 > + for (uint32_t idx = 0; idx < length; ++idx) { Rename it to index. > Source/JavaScriptCore/wasm/WasmInstance.cpp:187 > + JSWebAssemblyInstance* jsInstance = owner<JSWebAssemblyInstance>(); > + JSWebAssemblyTable* jsTable = jsInstance->table(tableIndex); We should put them out of the loop. > Source/JavaScriptCore/wasm/WasmInstance.cpp:199 > + JSGlobalObject* globalObject = jsInstance->globalObject(); > + VM& vm = globalObject->vm(); Ditto. Let's put them out of the loop. > Source/JavaScriptCore/wasm/WasmInstance.cpp:246 > +void Instance::tableInit(uint32_t dstOffset, uint32_t srcOffset, uint32_t len, uint32_t elementIndex, uint32_t tableIndex) len => length.
Created attachment 415850 [details] Patch
Added test in .wasm binary format because our wabt harness can't emit invalid wasm files.
Comment on attachment 415850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415850&action=review r=me with one important optimization. > Source/JavaScriptCore/wasm/WasmInstance.cpp:77 > + m_passiveElements.resize(m_module->moduleInformation().elementCount()); > + for (unsigned elementIndex = 0; elementIndex < m_passiveElements.size(); ++elementIndex) { > + const auto& element = m_module->moduleInformation().elements[elementIndex]; > + if (element.isPassive()) > + m_passiveElements[elementIndex] = element; > + } I think m_passiveElements can be a BitVector, and this is better in terms of memory. m_module is held by WasmInstance. And we can get corresponding element by m_module->moduleInformation().elements[index]. So, only thing we would like to know is that the given passive element is dropped or not. Can you change it to save memory? WasmInstance is kept alive so long as wasm code is running. So saving memory is very important. > Source/JavaScriptCore/wasm/WasmInstance.h:233 > + Vector<Optional<Element>> m_passiveElements; Let's make it BitVector.
Comment on attachment 415850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=415850&action=review > Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:610 > + RELEASE_ASSERT(element.isActive()); I don't think this is necessary or making it to ASSERT. forEachActiveElement is explicitly saying this.
Created attachment 415968 [details] Patch
Comment on attachment 415968 [details] Patch r=me
Committed r270689: <https://trac.webkit.org/changeset/270689> All reviewed patches have been landed. Closing bug and clearing flags on attachment 415968 [details].