...
Created attachment 383316 [details] Patch
Comment on attachment 383316 [details] Patch Attachment 383316 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13241154 New failing tests: wasm.yaml/wasm/stress/wasm-wasm-call-many-return-types-on-stack-no-args.js.default-wasm wasm.yaml/wasm/function-tests/stack-overflow.js.default-wasm wasm.yaml/wasm/stress/wasm-js-call-many-return-types-on-stack-no-args.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/many-arguments-to-function.js.wasm-eager wasm.yaml/wasm/function-tests/many-arguments-to-function.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/many-args-tail-call-sp-restored.js.default-wasm wasm.yaml/wasm/stress/wasm-wasm-call-indirect-many-return-types-on-stack.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/add-12.js.wasm-no-tls-context wasm.yaml/wasm/stress/wasm-wasm-call-indirect-many-return-types-on-stack.js.wasm-slow-memory wasm.yaml/wasm/stress/wasm-wasm-call-indirect-many-return-types-on-stack.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/many-args-tail-call-sp-restored.js.wasm-eager wasm.yaml/wasm/stress/wasm-js-call-many-return-types-on-stack-no-args.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/stress/wasm-wasm-call-indirect-many-return-types-on-stack.js.wasm-eager wasm.yaml/wasm/function-tests/many-args-tail-call-sp-restored.js.wasm-slow-memory wasm.yaml/wasm/stress/wasm-wasm-call-indirect-many-return-types-on-stack.js.wasm-eager-jettison wasm.yaml/wasm/stress/wasm-js-call-many-return-types-on-stack-no-args.js.default-wasm wasm.yaml/wasm/stress/wasm-js-call-many-return-types-on-stack-no-args.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/add-12.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/many-arguments-to-function.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/stress/wasm-js-call-many-return-types-on-stack-no-args.js.wasm-slow-memory wasm.yaml/wasm/stress/wasm-wasm-call-many-return-types-on-stack-no-args.js.wasm-no-tls-context wasm.yaml/wasm/stress/wasm-wasm-call-many-return-types-on-stack-no-args.js.wasm-slow-memory wasm.yaml/wasm/stress/wasm-wasm-call-indirect-many-return-types-on-stack.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-eager wasm.yaml/wasm/function-tests/add-12.js.wasm-slow-memory wasm.yaml/wasm/stress/wasm-js-call-many-return-types-on-stack-no-args.js.wasm-eager wasm.yaml/wasm/function-tests/add-12.js.wasm-eager wasm.yaml/wasm/function-tests/many-args-tail-call-sp-restored.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/add-12.js.default-wasm wasm.yaml/wasm/function-tests/many-args-tail-call-sp-restored.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/stress/wasm-wasm-call-many-return-types-on-stack-no-args.js.wasm-collect-continuously wasm.yaml/wasm/stress/wasm-js-call-many-return-types-on-stack-no-args.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/many-args-tail-call-sp-restored.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/many-arguments-to-function.js.wasm-no-tls-context wasm.yaml/wasm/function-tests/many-arguments-to-function.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/many-arguments-to-function.js.wasm-slow-memory wasm.yaml/wasm/stress/wasm-wasm-call-many-return-types-on-stack-no-args.js.wasm-eager-jettison wasm.yaml/wasm/function-tests/many-args-tail-call-sp-restored.js.wasm-collect-continuously wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-slow-memory wasm.yaml/wasm/function-tests/add-12.js.wasm-eager-jettison wasm.yaml/wasm/stress/wasm-wasm-call-many-return-types-on-stack-no-args.js.wasm-eager wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-eager-jettison wasm.yaml/wasm/stress/wasm-wasm-call-many-return-types-on-stack-no-args.js.wasm-no-cjit-yes-tls-context wasm.yaml/wasm/function-tests/stack-overflow.js.wasm-collect-continuously wasm.yaml/wasm/stress/wasm-wasm-call-indirect-many-return-types-on-stack.js.default-wasm wasm.yaml/wasm/function-tests/many-arguments-to-function.js.default-wasm wasm.yaml/wasm/function-tests/add-12.js.wasm-collect-continuously
Comment on attachment 383316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383316&action=review Tests are broken > Source/JavaScriptCore/ChangeLog:9 > + - Removing redundant moves when materializing constants You should also say how, not just what > Source/JavaScriptCore/ChangeLog:13 > + - Delay holding the lock in LLintPlan::compileFunction, since we do not need to hold it while compiling. I think you meant to say compiling the entrypoint > Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp:85 > entry.function = m_createEmbedderWrapper(*entry.jit, signature, &m_unlinkedWasmToWasmCalls[functionIndex], m_moduleInformation.get(), m_mode, functionIndex); We’re sure it’s ok to call this concurrently?
Created attachment 383387 [details] Patch
Comment on attachment 383316 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383316&action=review >> Source/JavaScriptCore/ChangeLog:9 >> + - Removing redundant moves when materializing constants > > You should also say how, not just what updated >> Source/JavaScriptCore/ChangeLog:13 >> + - Delay holding the lock in LLintPlan::compileFunction, since we do not need to hold it while compiling. > > I think you meant to say compiling the entrypoint fixed >> Source/JavaScriptCore/wasm/WasmLLIntPlan.cpp:85 >> entry.function = m_createEmbedderWrapper(*entry.jit, signature, &m_unlinkedWasmToWasmCalls[functionIndex], m_moduleInformation.get(), m_mode, functionIndex); > > We’re sure it’s ok to call this concurrently? yeah, it's fine
Comment on attachment 383387 [details] Patch Attachment 383387 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/13243998 New failing tests: stress/llint-put-to-scope-global-cache-watchpoint-invalidate.js.dfg-eager-no-cjit-validate
Created attachment 383731 [details] Patch
Created attachment 383745 [details] Patch
Created attachment 383764 [details] Patch Fix iOS build
Created attachment 383771 [details] Patch Change LLIntGenerator::Stack size to 16
Comment on attachment 383771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383771&action=review > Source/JavaScriptCore/ChangeLog:14 > + to work, we still allocate the stack slot for the temporaries, since we have to materialize them in before loops and branches. "them in" => "them" > Source/JavaScriptCore/bytecompiler/Label.h:131 > + typedef Vector<int, 8> JumpVector; typedef => using > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:52 > + using Stack = Vector<ExpressionType, 16, UnsafeVectorOverflow>; Does UnsafeVectorOverflow actually make a difference? > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:154 > + if (dst == src) > + continue; Does this actually happen? > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:165 > + void didPopValueFromStack() { --m_stackSize; } Why can't we make this a function of m_expressionStack and the reserved callee register size? That feels more robust than doing it this way, especially since there are already places we map from a stack index to a local. > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:312 > + if (expression != slot) { why not just check if it's a constant? > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:317 > + } can we write the code where we assert each non-constant on the stack is the local we expect it to be? Otherwise, this code will always feel fishy. Maybe we can have a "checkConsistency" which does that? > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:326 > + static constexpr bool emptyValueIsZero = false; this looks wrong :-) > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:384 > + // This function is setups the stack layout for calls. The desired stack layout is: "is setups" => "sets up" > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:580 > + nit: revert > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:694 > + for (int i = expressionStack.size(); i > 0; --i) { nit: same comment about the loop. Let's not use int > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:784 > + // TODO: don't emit jump for empty else TODO => FIXME and please file a bug and link it here if this is something that should be done.
Comment on attachment 383771 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383771&action=review >> Source/JavaScriptCore/bytecompiler/Label.h:131 >> + typedef Vector<int, 8> JumpVector; > > typedef => using Oops, I just moved it, will fix. >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:52 >> + using Stack = Vector<ExpressionType, 16, UnsafeVectorOverflow>; > > Does UnsafeVectorOverflow actually make a difference? I haven't measured, but I saw no reason to have overflow checks since we don't use it for arbitrary access. >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:154 >> + continue; > > Does this actually happen? It used to, it might not happen anymore. I can remove it. >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:165 >> + void didPopValueFromStack() { --m_stackSize; } > > Why can't we make this a function of m_expressionStack and the reserved callee register size? > That feels more robust than doing it this way, especially since there are already places we map from a stack index to a local. I don't follow... what do you mean by a function of m_expressionStack? We create the values that go into the expression stack based on m_stackSize. >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:312 >> + if (expression != slot) { > > why not just check if it's a constant? Because we also delay setLocal, in which case the VirtualRegister will point to a local instead of a stack slot >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:317 >> + } > > can we write the code where we assert each non-constant on the stack is the local we expect it to be? Otherwise, this code will always feel fishy. Maybe we can have a "checkConsistency" which does that? That's already what this function does, right? We traverse the whole expression stack, and we must either contain the expected virtual register (i.e. the one that'd point at that stack slot), a constant, or a wasm local (i.e. a delayed get_local) >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:326 >> + static constexpr bool emptyValueIsZero = false; > > this looks wrong :-) oops haha, I copied it from WTF/HashTraits and modified the zero value, but not the flag... I'll update. I don't think it's broken though, just bad. >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:384 >> + // This function is setups the stack layout for calls. The desired stack layout is: > > "is setups" => "sets up" will fix. >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:784 >> + // TODO: don't emit jump for empty else > > TODO => FIXME and please file a bug and link it here if this is something that should be done. Oops, I forgot this comment, I actually already did that.
Created attachment 384127 [details] Patch
Comment on attachment 384127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384127&action=review r=me > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:360 > + }); nit: maybe checkConsistency after too? > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:363 > + void materializeLocals(Stack& expressionStack, size_t newStackSize) put inside splitStack? Or maybe call this materializeLocalsAfterSplit and name "newStackSize" => "splitStackSize"? > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:403 > + unsigned m_topLevelStackSize; this seems completely unused (except setting it somewhere) > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:783 > + materializeLocals(enclosingStack, newStack.size()); why not put materializeLocals into splitStack? > Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:959 > +#if !ASSERT_DISABLED > + ASSERT(unreachable || tmp == expressionStack[i]); > +#endif no need for !ASSERT_DISABLED here. Also, you should just ASSERT_UNUSED and remove the UNUSED_PARAM above
Comment on attachment 384127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384127&action=review Thanks for the review! >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:360 >> + }); > > nit: maybe checkConsistency after too? Good call >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:363 >> + void materializeLocals(Stack& expressionStack, size_t newStackSize) > > put inside splitStack? Or maybe call this materializeLocalsAfterSplit and name "newStackSize" => "splitStackSize"? I added a custom splitStack to LLIntGenerator that does it. >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:403 >> + unsigned m_topLevelStackSize; > > this seems completely unused (except setting it somewhere) you're right. I added it but removed its uses while refactoring the patch. I'll remove it. >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:783 >> + materializeLocals(enclosingStack, newStack.size()); > > why not put materializeLocals into splitStack? done >> Source/JavaScriptCore/wasm/WasmLLIntGenerator.cpp:959 >> +#endif > > no need for !ASSERT_DISABLED here. Also, you should just ASSERT_UNUSED and remove the UNUSED_PARAM above good point. There was some more code in there at some point, but it doesn't make sense anymore. I'll fix it.
Created attachment 384190 [details] Patch for landing
Comment on attachment 384190 [details] Patch for landing Clearing flags on attachment: 384190 Committed r252800: <https://trac.webkit.org/changeset/252800>
All reviewed patches have been landed. Closing bug.
<rdar://problem/57440426>