Bug 204092 - [WebAssembly] Improve Wasm::LLIntGenerator
Summary: [WebAssembly] Improve Wasm::LLIntGenerator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
Depends on:
Blocks: 204474
  Show dependency treegraph
 
Reported: 2019-11-11 16:53 PST by Tadeu Zagallo
Modified: 2019-11-22 14:14 PST (History)
8 users (show)

See Also:


Attachments
Patch (32.43 KB, patch)
2019-11-11 16:55 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (33.73 KB, patch)
2019-11-12 14:51 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (64.57 KB, patch)
2019-11-17 21:45 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (64.53 KB, patch)
2019-11-18 07:30 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (64.53 KB, patch)
2019-11-18 11:24 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (64.53 KB, patch)
2019-11-18 11:48 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (69.83 KB, patch)
2019-11-21 23:44 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch for landing (69.58 KB, patch)
2019-11-22 13:28 PST, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-11-11 16:53:12 PST
...
Comment 1 Tadeu Zagallo 2019-11-11 16:55:50 PST
Created attachment 383316 [details]
Patch
Comment 2 EWS Watchlist 2019-11-11 19:24:56 PST
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 3 Saam Barati 2019-11-12 08:43:54 PST
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?
Comment 4 Tadeu Zagallo 2019-11-12 14:51:42 PST
Created attachment 383387 [details]
Patch
Comment 5 Tadeu Zagallo 2019-11-12 15:06:38 PST
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 6 EWS Watchlist 2019-11-12 17:21:08 PST
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
Comment 7 Tadeu Zagallo 2019-11-17 21:45:16 PST
Created attachment 383731 [details]
Patch
Comment 8 Tadeu Zagallo 2019-11-18 07:30:50 PST
Created attachment 383745 [details]
Patch
Comment 9 Tadeu Zagallo 2019-11-18 11:24:55 PST
Created attachment 383764 [details]
Patch

Fix iOS build
Comment 10 Tadeu Zagallo 2019-11-18 11:48:22 PST
Created attachment 383771 [details]
Patch

Change LLIntGenerator::Stack size to 16
Comment 11 Saam Barati 2019-11-18 16:58:19 PST
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 12 Tadeu Zagallo 2019-11-18 18:12:29 PST
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.
Comment 13 Tadeu Zagallo 2019-11-21 23:44:09 PST
Created attachment 384127 [details]
Patch
Comment 14 Saam Barati 2019-11-22 12:03:37 PST
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 15 Tadeu Zagallo 2019-11-22 13:10:30 PST
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.
Comment 16 Tadeu Zagallo 2019-11-22 13:28:25 PST
Created attachment 384190 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2019-11-22 14:13:35 PST
Comment on attachment 384190 [details]
Patch for landing

Clearing flags on attachment: 384190

Committed r252800: <https://trac.webkit.org/changeset/252800>
Comment 18 WebKit Commit Bot 2019-11-22 14:13:37 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Radar WebKit Bug Importer 2019-11-22 14:14:21 PST
<rdar://problem/57440426>