Bug 201799 – Change WebAssembly calling conventions

• Status: REOPENED
• Component: JavaScriptCore

[Tadeu Zagallo]

...

[Tadeu Zagallo]

Created attachment 378800 [details]
WIP

[Tadeu Zagallo]

It seems like my last refactoring broke everything, but I'll leave this WIP patch here for now.

[Tadeu Zagallo]

Created attachment 378807 [details]
Patch

[Tadeu Zagallo]

Created attachment 378828 [details]
Patch

Attaching the rebased patch, but it seems that tests are not starting locally. Investigating.

[EWS Watchlist]

Comment on attachment 378828 [details]
Patch

Attachment 378828 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/13034878

New failing tests:
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above-no-consts.wasm)-wasm-collect-continuously
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-multiple-enclosed-contexts.wasm)-wasm-no-cjit-yes-tls-context
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above-no-consts.wasm)-wasm-no-cjit-yes-tls-context
wasm.yaml/wasm/js-api/Instance.imports.exports.unicode.js.wasm-collect-continuously
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-multiple-enclosed-contexts.wasm)-default-wasm
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above.wasm)-wasm-collect-continuously
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above-no-consts.wasm)-wasm-eager-jettison
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above-no-consts.wasm)-wasm-no-call-ic
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop.wasm)-wasm-slow-memory
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-multiple-enclosed-contexts.wasm)-wasm-eager
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop.wasm)-default-wasm
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above.wasm)-wasm-eager-jettison
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above-no-consts.wasm)-wasm-eager
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above.wasm)-wasm-no-tls-context
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above.wasm)-wasm-eager
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop.wasm)-wasm-no-call-ic
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-multiple-enclosed-contexts.wasm)-wasm-no-tls-context
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop.wasm)-wasm-no-cjit-yes-tls-context
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop.wasm)-wasm-collect-continuously
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-multiple-enclosed-contexts.wasm)-wasm-slow-memory
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-multiple-enclosed-contexts.wasm)-wasm-collect-continuously
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-multiple-enclosed-contexts.wasm)-wasm-no-air
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop.wasm)-wasm-eager-jettison
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above.wasm)-wasm-slow-memory
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above-no-consts.wasm)-wasm-no-air
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above-no-consts.wasm)-default-wasm
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above.wasm)-default-wasm
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop.wasm)-wasm-no-air
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-multiple-enclosed-contexts.wasm)-wasm-eager-jettison
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above-no-consts.wasm)-wasm-no-tls-context
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop.wasm)-wasm-eager
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above.wasm)-wasm-no-cjit-yes-tls-context
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop.wasm)-wasm-no-tls-context
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above.wasm)-wasm-no-air
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above.wasm)-wasm-no-call-ic
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-inner-loop-branch-above-no-consts.wasm)-wasm-slow-memory
wasm.yaml/wasm/wast-tests/harness.js.(osr-entry-multiple-enclosed-contexts.wasm)-wasm-no-call-ic

[Tadeu Zagallo]

Created attachment 378844 [details]
Patch

Fix OMG osr entry.

[Yusuke Suzuki]

Comment on attachment 378844 [details]
Patch

Can you also ensure that this change keeps SamplingProfiler. work? SamplingProfiler relies on that WasmCallee exists on Callee place.

[Tadeu Zagallo]

Created attachment 378859 [details]
Patch

Fix WebKit build.

[Tadeu Zagallo]

Created attachment 378861 [details]
Patch

Fix CMake build.

[Tadeu Zagallo]

Created attachment 378867 [details]
Patch

Fix CMake build (take 2).

[Tadeu Zagallo]

Created attachment 378868 [details]
Patch

Fix CMake build (take 3).

[Tadeu Zagallo]

Created attachment 378869 [details]
Patch

Fix CMake build (take 4).

[Tadeu Zagallo]

Comment on attachment 378869 [details]
Patch

I've confirmed that the sampling profiler still works as expected and the bots seem happy (the wincairo failure seems like an infra failure). I think this is actually good for review now.

[Saam Barati]

Comment on attachment 378869 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378869&action=review

> Source/JavaScriptCore/ChangeLog:16
> +        - Callee writes to call frame: wasm-to-wasm stub, JS-to-wasm stub (callee frame), wasm-to-JS stub, OMG osr entry

nit: "wasm-to-wasm stub" => "inter-module wasm-to-wasm stub"
(since you specify intra above)

> Source/JavaScriptCore/ChangeLog:18
> +        Additionally, also change it so that the callee keeps track of its callers, instead of having a global mapping

"also change" => maybe something like "this patch also changes"

> Source/JavaScriptCore/wasm/WasmBBQPlan.cpp:331
> +

nit: no newline here

> Source/JavaScriptCore/wasm/WasmBBQPlan.h:142
> +    const Vector<RefPtr<BBQCallee>>* m_callees { nullptr };

this is never actually null AFAICT. Why not make it a reference?

> Source/JavaScriptCore/wasm/WasmBinding.cpp:71
> +    // At this point, we have been called, so ReturnPC is on the stack, but have not yet pushed the base pointer,

"but have" => "but we have"
"base pointer" => "frame pointer"

> Source/JavaScriptCore/wasm/WasmBinding.cpp:73
> +    jit.storePtr(callee, JIT::Address(JIT::stackPointerRegister, CallFrameSlot::callee * sizeof(Register) - sizeof(CPURegister)));

why sizeof(Register) and sizeof(CPURegister) on same line? Why not use one?

> Source/JavaScriptCore/wasm/WasmOMGForOSREntryPlan.cpp:109
> +            BBQCallee& targetCallee = m_codeBlock->wasmBBQCalleeFromFunctionIndexSpace(call.targetFunctionIndexSpace);

why just BBQ? What if it's already OMG compiled?

> Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:122
> +            BBQCallee& targetCallee = m_codeBlock->wasmBBQCalleeFromFunctionIndexSpace(call.targetFunctionIndexSpace);

why BBQ callee? What if the target is already OMG compiled?

> Source/JavaScriptCore/wasm/WasmOperations.cpp:203
> +    *(stackPointer + CallFrameSlot::callee - 1) = CalleeBits::boxWasm(&osrEntryCallee);

this math relies on sizeof void*. You should instead rely on CPURegister, since this code is wrong for arm64_32.

So something like "CPURegister* stackPointer = ...;"

[Tadeu Zagallo]

Comment on attachment 378869 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378869&action=review

Thanks for reviewing it!

>> Source/JavaScriptCore/ChangeLog:16
>> +        - Callee writes to call frame: wasm-to-wasm stub, JS-to-wasm stub (callee frame), wasm-to-JS stub, OMG osr entry
> 
> nit: "wasm-to-wasm stub" => "inter-module wasm-to-wasm stub"
> (since you specify intra above)

Fixed.

>> Source/JavaScriptCore/ChangeLog:18
>> +        Additionally, also change it so that the callee keeps track of its callers, instead of having a global mapping
> 
> "also change" => maybe something like "this patch also changes"

Fixed.

>> Source/JavaScriptCore/wasm/WasmBBQPlan.h:142
>> +    const Vector<RefPtr<BBQCallee>>* m_callees { nullptr };
> 
> this is never actually null AFAICT. Why not make it a reference?

Good point, fixed.

>> Source/JavaScriptCore/wasm/WasmBinding.cpp:71
>> +    // At this point, we have been called, so ReturnPC is on the stack, but have not yet pushed the base pointer,
> 
> "but have" => "but we have"
> "base pointer" => "frame pointer"

Fixed.

>> Source/JavaScriptCore/wasm/WasmBinding.cpp:73
>> +    jit.storePtr(callee, JIT::Address(JIT::stackPointerRegister, CallFrameSlot::callee * sizeof(Register) - sizeof(CPURegister)));
> 
> why sizeof(Register) and sizeof(CPURegister) on same line? Why not use one?

Because they might be different. sizeof(Register) is always 8 and sizeof(CPURegister) might be 4 or 8 depending on the arch. ReturnPC is sizeof(CPURegister) while callee, codeblock and argumentCount are sizeof(Register).

>> Source/JavaScriptCore/wasm/WasmOMGForOSREntryPlan.cpp:109
>> +            BBQCallee& targetCallee = m_codeBlock->wasmBBQCalleeFromFunctionIndexSpace(call.targetFunctionIndexSpace);
> 
> why just BBQ? What if it's already OMG compiled?

We call addAndLinkCaller below, which checks if the callee has a replacement. In which case, it will link it against the OMG callee. But it always the BBQ callee who holds the callers.

>> Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:122
>> +            BBQCallee& targetCallee = m_codeBlock->wasmBBQCalleeFromFunctionIndexSpace(call.targetFunctionIndexSpace);
> 
> why BBQ callee? What if the target is already OMG compiled?

Replied above.

>> Source/JavaScriptCore/wasm/WasmOperations.cpp:203
>> +    *(stackPointer + CallFrameSlot::callee - 1) = CalleeBits::boxWasm(&osrEntryCallee);
> 
> this math relies on sizeof void*. You should instead rely on CPURegister, since this code is wrong for arm64_32.
> 
> So something like "CPURegister* stackPointer = ...;"

Fixed.

[Tadeu Zagallo]

Created attachment 378986 [details]
Patch

[Saam Barati]

Comment on attachment 378986 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378986&action=review

> Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:123
> +            BBQCallee& targetCallee = m_codeBlock->wasmBBQCalleeFromFunctionIndexSpace(call.targetFunctionIndexSpace);
> +            targetCallee.addAndLinkCaller(holder, linkBuffer, call.unlinkedMoveAndCall);

this targetCallee is different than the one below. The below is what we're compiling. This targetCallee is what OMG is going to call. It seems wrong to make OMG compile always call BBQ...

[Saam Barati]

Comment on attachment 378986 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378986&action=review

r=me

>> Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:123
>> +            targetCallee.addAndLinkCaller(holder, linkBuffer, call.unlinkedMoveAndCall);
> 
> this targetCallee is different than the one below. The below is what we're compiling. This targetCallee is what OMG is going to call. It seems wrong to make OMG compile always call BBQ...

I see. addAndLinkCaller looks for a replacement. Seems like kinda a weird paradigm, but it's OK

[Tadeu Zagallo]

Comment on attachment 378986 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378986&action=review

>>> Source/JavaScriptCore/wasm/WasmOMGPlan.cpp:123
>>> +            targetCallee.addAndLinkCaller(holder, linkBuffer, call.unlinkedMoveAndCall);
>> 
>> this targetCallee is different than the one below. The below is what we're compiling. This targetCallee is what OMG is going to call. It seems wrong to make OMG compile always call BBQ...
> 
> I see. addAndLinkCaller looks for a replacement. Seems like kinda a weird paradigm, but it's OK

We could do it explicitly, by passing targetCallee.replacement(), but I thought that seemed unnecessary. I wanted to keep all callers in the BBQCallee so that we don't need to iterate through all callees.

[Tadeu Zagallo]

Created attachment 379003 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 379003 [details]
Patch for landing

Clearing flags on attachment: 379003

Committed r250002: <https://trac.webkit.org/changeset/250002>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/55459868>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 201943