Bug 165163

Created attachment 295673 [details] WIP It probably doesn't compile yet.

Created attachment 295691 [details] WIP it compiles

Comment on attachment 295691 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=295691&action=review > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:202 > + // FIXME: how do we make sure this is the first thing to execute? Remove this. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:205 > + getCalleePatchpoint->effects = Effects::none(); Remove this.

Created attachment 295904 [details] RFC WIP

Comment on attachment 295904 [details] RFC WIP View in context: https://bugs.webkit.org/attachment.cgi?id=295904&action=review > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:236 > + Value* offsetOfCodeBlock = m_currentBlock->appendNew<Const64Value>(m_proc, Origin(), CallFrameSlot::codeBlock * sizeof(Register)); > + m_currentBlock->appendNew<MemoryValue>(m_proc, Store, Origin(), > + m_currentBlock->appendNew<Const64Value>(m_proc, Origin(), 0), > + m_currentBlock->appendNew<Value>(m_proc, Add, Origin(), framePointer, offsetOfCodeBlock)); We probably shouldn't do this, but it will mean I need to audit the stack walking behavior to make sure it doesn't take a non-null codeblock to mean that the CodeBlock is a valid value.

Comment on attachment 295904 [details] RFC WIP View in context: https://bugs.webkit.org/attachment.cgi?id=295904&action=review > Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:68 > + AuxiliaryBarrier<JSWebAssemblyCallee**> m_callees; We could probably allocate a variable sized cell and just have this buffer be at the end of this cell's fields similar to lexical environments

Comment on attachment 295904 [details] RFC WIP View in context: https://bugs.webkit.org/attachment.cgi?id=295904&action=review > Source/JavaScriptCore/wasm/WasmPlan.h:40 > +class MarkedArgumentBuffer; This should be deleted.

Comment on attachment 295904 [details] RFC WIP View in context: https://bugs.webkit.org/attachment.cgi?id=295904&action=review > Source/JavaScriptCore/jsc.cpp:2540 > + return JSValue::decode(vmEntryToWasm(callee->jsEntryPoint(), vm, &protoCallFrame)); FYI I'm renaming this to jsToWasmEntryPoint to be clearer. > Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:58 > + return m_callees.get()[calleeIndex]; .at() wouldn't remove the need for the assert. > Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:68 > + AuxiliaryBarrier<JSWebAssemblyCallee**> m_callees; Can't this be a Vector<WriteBarrier> ? Seems like the code would be simpler that way.

Comment on attachment 295904 [details] RFC WIP View in context: https://bugs.webkit.org/attachment.cgi?id=295904&action=review >>> Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:68 >>> + AuxiliaryBarrier<JSWebAssemblyCallee**> m_callees; >> >> We could probably allocate a variable sized cell and just have this buffer be at the end of this cell's fields similar to lexical environments > > Can't this be a Vector<WriteBarrier> ? Seems like the code would be simpler that way. I'm just going to make it inline at the end of this object.

Created attachment 295924 [details] WIP I think this might be done, I just want to read it over and write a changelog.

Created attachment 295925 [details] WIP fixed a comment.

Created attachment 295985 [details] patch

Comment on attachment 295985 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295985&action=review Looks good. > Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:81 > + unsigned m_calleeCount; So there's usually no maker at the end here? It's valid C to put JSWebAssemblyCallee m_callee[];

Comment on attachment 295985 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295985&action=review r=me with some comments. > Source/JavaScriptCore/jsc.cpp:-2588 > - if (!plan.compiledFunction(i)) > - dataLogLn("failed to compile function at index", i); Why'd you delete this? > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:213 > + PatchpointValue* getCalleePatchpoint = m_currentBlock->appendNew<B3::PatchpointValue>(m_proc, Int64, Origin()); I think this should all be done in some function in WasmCallingConvention.h. My hope was to make it easier to manage all the various calling convention stuff if it's all there. > Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:229 > + m_currentBlock->appendNew<MemoryValue>(m_proc, Store, Origin(), You can make sure other potentially trapping memory ops are not move over this with: m_currentBlock->appendNew<MemoryValue>(m_proc, traps(Store), Origin(), ... I guess I forgot to add that to the wasm memory opcodes. I'll make a patch for that now.

Comment on attachment 295985 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295985&action=review >> Source/JavaScriptCore/jsc.cpp:-2588 >> - dataLogLn("failed to compile function at index", i); > > Why'd you delete this? I thought it was redundant with ``` if (plan.failed()) CRASH(); ``` and ``` if (plan.compiledFunctionCount() != functionCount) CRASH(); ``` Is it not? >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:213 >> + PatchpointValue* getCalleePatchpoint = m_currentBlock->appendNew<B3::PatchpointValue>(m_proc, Int64, Origin()); > > I think this should all be done in some function in WasmCallingConvention.h. My hope was to make it easier to manage all the various calling convention stuff if it's all there. Ok, will do. >> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:229 >> + m_currentBlock->appendNew<MemoryValue>(m_proc, Store, Origin(), > > You can make sure other potentially trapping memory ops are not move over this with: m_currentBlock->appendNew<MemoryValue>(m_proc, traps(Store), Origin(), ... > > I guess I forgot to add that to the wasm memory opcodes. I'll make a patch for that now. Actually, after reading though some B3 code, it would be invalid for this to happen. All trapping memory accesses claim to read the entire heap, so it would be invalid for this store to not execute before those. >> Source/JavaScriptCore/wasm/js/JSWebAssemblyModule.h:81 >> + unsigned m_calleeCount; > > So there's usually no maker at the end here? It's valid C to put JSWebAssemblyCallee m_callee[]; We don't usually do this with the various cells that are implemented this way. I'm not sure why.

Comment on attachment 295985 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=295985&action=review >>> Source/JavaScriptCore/jsc.cpp:-2588 >>> - dataLogLn("failed to compile function at index", i); >> >> Why'd you delete this? > > I thought it was redundant with > ``` > if (plan.failed()) CRASH(); > ``` > and > ``` > if (plan.compiledFunctionCount() != functionCount) CRASH(); > ``` > > Is it not? Ah I missed the plan.failed(). Yeah, this probably doesn't do anything.

Created attachment 296100 [details] patch for landing

landed in: https://trac.webkit.org/changeset/209312