[wasm] Add support for compiling WASM_INTERPRETER_ENTRY stubs

[wasm] Add support for compiling WASM_INTERPRETER_ENTRY stubs

Also, add a runtime function to call the interpreter, passing a
stack-allocated buffer holding the arguments.
The WASM_INTERPRETER_ENTRY stub allocates the stack slot for the
arguments, fills it, and calls to the wasm interpreter.
It's abi is compatible with WASM functions, such that we can just
replace a call to a WASM_FUNCTION with a call to
WASM_INTERPRETER_ENTRY.
See tracking bug to get the overall picture.

BUG=v8:5822
R=titzer@chromium.org

Review-Url: https://codereview.chromium.org/2619803004
Cr-Commit-Position: refs/heads/master@{#42242}
Committed: https://chromium.googlesource.com/v8/v8/+/a2efde46a1678e67ad20fc5ccda471beb5e24a4d

[Clemens Hammacher, 2017-01-10 11:09:20]

The CQ bit was checked by clemensh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 11:09:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-10 11:43:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-10 11:43:46]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-01-10 19:00:51]

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2939: Node* arg_buffer =
build_runtime_call(Runtime::kGetWasmInterpreterBuffer);
I think we can put this buffer into the stack frame of the caller. We already
have a mechanism to allocate a single stack slot which we could generalize to a
fixed-size buffer. That will also be safer for reentrancy.

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2955: if (is_i64_as_two_params) {
I think this might be backward for big-endian platforms.

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2963: offset += 1 <<
ElementSizeLog2Of(wasm::kWasmI32);
sizeof(int32_t) is also fine.

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler.h
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.h:107: Handle<Code> CompileWasmToInterpreter(Isolate*
isolate, uint32_t func_index,
As in previous CL, a better name is CompileWasmInterpreterEntry.

[titzer, 2017-01-10 19:02:47]

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2939: Node* arg_buffer =
build_runtime_call(Runtime::kGetWasmInterpreterBuffer);
On 2017/01/10 19:00:51, titzer wrote:
> I think we can put this buffer into the stack frame of the caller. We already
> have a mechanism to allocate a single stack slot which we could generalize to
a
> fixed-size buffer. That will also be safer for reentrancy.

Actually, now that I think about it, we might be able to use a special call
descriptor to force all the arguments onto the stack, and then the TurboFan
backend will just do all the moves for us.

[Clemens Hammacher, 2017-01-11 14:51:33]

Description was changed from

==========
[wasm] Add support for compiling WASM_TO_INTERPRETER stubs

Also, add runtime functions to (1) get a buffer from the interpreter to
store the call arguments, and (2) call the interpreter.
The WASM_TO_INTERPRETER stubs use these runtime functions to redirect a
call from WASM code to the wasm interpreter, implemented in C++.
See tracking bug to get the overall picture.

BUG=v8:5822
R=titzer@chromium.org
==========

to

==========
[wasm] Add support for compiling WASM_INTERPRETER_ENTRY stubs

Also, add runtime functions to (1) get a buffer from the interpreter to
store the call arguments, and (2) call the interpreter.
The WASM_INTERPRETER_ENTRY stubs use these runtime functions to redirect
a call from WASM code to the wasm interpreter, implemented in C++.
See tracking bug to get the overall picture.

BUG=v8:5822
R=titzer@chromium.org
==========

[Clemens Hammacher, 2017-01-11 15:03:39]

Description was changed from

==========
[wasm] Add support for compiling WASM_INTERPRETER_ENTRY stubs

Also, add runtime functions to (1) get a buffer from the interpreter to
store the call arguments, and (2) call the interpreter.
The WASM_INTERPRETER_ENTRY stubs use these runtime functions to redirect
a call from WASM code to the wasm interpreter, implemented in C++.
See tracking bug to get the overall picture.

BUG=v8:5822
R=titzer@chromium.org
==========

to

==========
[wasm] Add support for compiling WASM_INTERPRETER_ENTRY stubs

Also, add a runtime function to call the interpreter, passing a
stack-allocated buffer holding the arguments.
The WASM_INTERPRETER_ENTRY stub allocates the stack slot for the
arguments, fills it, and calls to the wasm interpreter.
It's abi is compatible with WASM functions, such that we can just
replace a call to a WASM_FUNCTION with a call to
WASM_INTERPRETER_ENTRY.
See tracking bug to get the overall picture.

BUG=v8:5822
R=titzer@chromium.org
==========

[Clemens Hammacher, 2017-01-11 15:05:16]

The CQ bit was checked by clemensh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 15:05:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Clemens Hammacher, 2017-01-11 15:05:29]

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler.cc
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2939: Node* arg_buffer =
build_runtime_call(Runtime::kGetWasmInterpreterBuffer);
On 2017/01/10 at 19:00:51, titzer wrote:
> I think we can put this buffer into the stack frame of the caller. We already
have a mechanism to allocate a single stack slot which we could generalize to a
fixed-size buffer. That will also be safer for reentrancy.

Done: http://crrev.com/2624183002

Using a special call descriptor does not work, since it must be abi-compatible
to WASM_CODE.

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2955: if (is_i64_as_two_params) {
On 2017/01/10 at 19:00:51, titzer wrote:
> I think this might be backward for big-endian platforms.

According to AddParameterNodes (line 2811) and BuildJSToWasmWrapper (line 2776),
the lower word always comes first in the parameters.

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.cc:2963: offset += 1 <<
ElementSizeLog2Of(wasm::kWasmI32);
On 2017/01/10 at 19:00:51, titzer wrote:
> sizeof(int32_t) is also fine.

Yes, but this way it is the same expression as 7 lines above.

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler.h
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/2619803004/diff/1/src/compiler/wasm-compiler....
src/compiler/wasm-compiler.h:107: Handle<Code> CompileWasmToInterpreter(Isolate*
isolate, uint32_t func_index,
On 2017/01/10 at 19:00:51, titzer wrote:
> As in previous CL, a better name is CompileWasmInterpreterEntry.

Done.

[commit-bot: I haz the power, 2017-01-11 15:37:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 15:37:38]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-01-11 15:52:34]

lgtm for now, excited to see how this story ends :)

https://codereview.chromium.org/2619803004/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2619803004/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2959: if (is_i64_as_two_params) {
As discussed in person, you'll have to check the endianness and store the upper
half first. Need to put a TODO here.

https://codereview.chromium.org/2619803004/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2976: arg_buffer,                              //
argument buffer
Can you add a TODO or other comment that we are passing the arg buffer
effectively as a SMI? That's fine on 32-bit (since the buffer is aligned and has
lower bits of 0), but on 64-bit it's actually an invalid SMI.

https://codereview.chromium.org/2619803004/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/2619803004/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.h:105: // Compiles a function which captures the
parameters and calls into the wasm
Can we reword this comment, since "capture" has a specific connotation for
closures...maybe just "Compiles a stub that redirects a call to a function to
the wasm interpreter".

https://codereview.chromium.org/2619803004/diff/20001/src/runtime/runtime-was...
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2619803004/diff/20001/src/runtime/runtime-was...
src/runtime/runtime-wasm.cc:160: CONVERT_ARG_HANDLE_CHECKED(Object,
arg_buffer_obj, 2);
Hmmm, not sure converting the argument buffer to Object is a good idea. You
maybe want to convert it to a SMI, but unchecked, since the SMI check will break
on 64-bit as mentioned previously.

[Clemens Hammacher, 2017-01-11 16:11:49]

The CQ bit was checked by clemensh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 16:11:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Clemens Hammacher, 2017-01-11 16:14:58]

The CQ bit was checked by clemensh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 16:15:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 16:42:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 16:42:51]

Dry run: This issue passed the CQ dry run.

[Clemens Hammacher, 2017-01-11 16:56:11]

https://codereview.chromium.org/2619803004/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.cc (right):

https://codereview.chromium.org/2619803004/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2959: if (is_i64_as_two_params) {
On 2017/01/11 at 15:52:34, titzer wrote:
> As discussed in person, you'll have to check the endianness and store the
upper half first. Need to put a TODO here.

Done.

https://codereview.chromium.org/2619803004/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.cc:2976: arg_buffer,                              //
argument buffer
On 2017/01/11 at 15:52:34, titzer wrote:
> Can you add a TODO or other comment that we are passing the arg buffer
effectively as a SMI? That's fine on 32-bit (since the buffer is aligned and has
lower bits of 0), but on 64-bit it's actually an invalid SMI.

I think IsSmi would actually return true, even though Smi::IsValid would return
false.
I added a comment to explain what we are doing here :)

https://codereview.chromium.org/2619803004/diff/20001/src/compiler/wasm-compi...
File src/compiler/wasm-compiler.h (right):

https://codereview.chromium.org/2619803004/diff/20001/src/compiler/wasm-compi...
src/compiler/wasm-compiler.h:105: // Compiles a function which captures the
parameters and calls into the wasm
On 2017/01/11 at 15:52:34, titzer wrote:
> Can we reword this comment, since "capture" has a specific connotation for
closures...maybe just "Compiles a stub that redirects a call to a function to
the wasm interpreter".

Done.

https://codereview.chromium.org/2619803004/diff/20001/src/runtime/runtime-was...
File src/runtime/runtime-wasm.cc (right):

https://codereview.chromium.org/2619803004/diff/20001/src/runtime/runtime-was...
src/runtime/runtime-wasm.cc:160: CONVERT_ARG_HANDLE_CHECKED(Object,
arg_buffer_obj, 2);
On 2017/01/11 at 15:52:34, titzer wrote:
> Hmmm, not sure converting the argument buffer to Object is a good idea. You
maybe want to convert it to a SMI, but unchecked, since the SMI check will break
on 64-bit as mentioned previously.

But what do I do with the Smi? I would just reinterpret_cast it as below.
I added another comment here, and CHECK(arg_buffer_obj->IsSmi()), but I don't
want to assign it to a Smi*, as it's not really a Smi.

[Clemens Hammacher, 2017-01-11 17:37:29]

The CQ bit was checked by clemensh@chromium.org

[Clemens Hammacher, 2017-01-11 17:37:29]

The patchset sent to the CQ was uploaded after l-g-t-m from titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2619803004/#ps60001
(title: "Change void* to uint8_t*")

[commit-bot: I haz the power, 2017-01-11 17:37:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 17:39:10]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484156249461230,
"parent_rev": "9e60e7ac365d45f3211314f1ecff395f418de770", "commit_rev":
"a2efde46a1678e67ad20fc5ccda471beb5e24a4d"}

[commit-bot: I haz the power, 2017-01-11 17:39:17]

Description was changed from

==========
[wasm] Add support for compiling WASM_INTERPRETER_ENTRY stubs

Also, add a runtime function to call the interpreter, passing a
stack-allocated buffer holding the arguments.
The WASM_INTERPRETER_ENTRY stub allocates the stack slot for the
arguments, fills it, and calls to the wasm interpreter.
It's abi is compatible with WASM functions, such that we can just
replace a call to a WASM_FUNCTION with a call to
WASM_INTERPRETER_ENTRY.
See tracking bug to get the overall picture.

BUG=v8:5822
R=titzer@chromium.org
==========

to

==========
[wasm] Add support for compiling WASM_INTERPRETER_ENTRY stubs

Also, add a runtime function to call the interpreter, passing a
stack-allocated buffer holding the arguments.
The WASM_INTERPRETER_ENTRY stub allocates the stack slot for the
arguments, fills it, and calls to the wasm interpreter.
It's abi is compatible with WASM functions, such that we can just
replace a call to a WASM_FUNCTION with a call to
WASM_INTERPRETER_ENTRY.
See tracking bug to get the overall picture.

BUG=v8:5822
R=titzer@chromium.org

Review-Url: https://codereview.chromium.org/2619803004
Cr-Commit-Position: refs/heads/master@{#42242}
Committed:
https://chromium.googlesource.com/v8/v8/+/a2efde46a1678e67ad20fc5ccda471beb5e...
==========

[commit-bot: I haz the power, 2017-01-11 17:39:18]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/a2efde46a1678e67ad20fc5ccda471beb5e...