Bug 204092 – [WebAssembly] Improve Wasm::LLIntGenerator

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 383316 [details]
Patch

[EWS Watchlist]

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

[Saam Barati]

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?

[Tadeu Zagallo]

Created attachment 383387 [details]
Patch

[Tadeu Zagallo]

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

[EWS Watchlist]

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

[Tadeu Zagallo]

Created attachment 383731 [details]
Patch

[Tadeu Zagallo]

Created attachment 383745 [details]
Patch

[Tadeu Zagallo]

Created attachment 383764 [details]
Patch

Fix iOS build

[Tadeu Zagallo]

Created attachment 383771 [details]
Patch

Change LLIntGenerator::Stack size to 16

[Saam Barati]

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.

[Tadeu Zagallo]

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.

[Tadeu Zagallo]

Created attachment 384127 [details]
Patch

[Saam Barati]

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

[Tadeu Zagallo]

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.

[Tadeu Zagallo]

Created attachment 384190 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 384190 [details]
Patch for landing

Clearing flags on attachment: 384190

Committed r252800: <https://trac.webkit.org/changeset/252800>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57440426>