FFI Compiler based on code stub assembler

FFI Compiler outline based on code stub assembler. We are looking to land this frame to allow specific type translation implementations to proceed in parallel.

BUG=v8:4456
Review-Url: https://codereview.chromium.org/2607993003
Cr-Commit-Position: refs/heads/master@{#42475}
Committed: https://chromium.googlesource.com/v8/v8/+/0ecc6b060045d617ac31d94816054d2a3cca7855

[mattloring, 2016-12-29 23:24:23]

Description was changed from

==========
FFI Compiler based on code stub assembler

BUG=
==========

to

==========
FFI Compiler based on code stub assembler

BUG=
==========

[mattloring, 2016-12-29 23:24:23]

mattloring@google.com changed reviewers:
+ ofrobots@google.com

[ofrobots, 2017-01-12 19:29:39]

On 2016/12/29 23:24:23, mattloring wrote:
> mailto:mattloring@google.com changed reviewers:
> + mailto:ofrobots@google.com

Looking like a good starting point.

[ofrobots, 2017-01-12 19:29:47]

https://codereview.chromium.org/2607993003/diff/80001/src/compiler/ffi-compil...
File src/compiler/ffi-compiler.cc (right):

https://codereview.chromium.org/2607993003/diff/80001/src/compiler/ffi-compil...
src/compiler/ffi-compiler.cc:45: UNREACHABLE();
Add a // TODO: needs to be implemented

https://codereview.chromium.org/2607993003/diff/80001/src/compiler/ffi-compil...
src/compiler/ffi-compiler.cc:51: UNREACHABLE();
Likewise.

[mattloring, 2017-01-12 19:40:11]

https://codereview.chromium.org/2607993003/diff/80001/src/compiler/ffi-compil...
File src/compiler/ffi-compiler.cc (right):

https://codereview.chromium.org/2607993003/diff/80001/src/compiler/ffi-compil...
src/compiler/ffi-compiler.cc:45: UNREACHABLE();
On 2017/01/12 19:29:47, ofrobots wrote:
> Add a // TODO: needs to be implemented

Done.

https://codereview.chromium.org/2607993003/diff/80001/src/compiler/ffi-compil...
src/compiler/ffi-compiler.cc:51: UNREACHABLE();
On 2017/01/12 19:29:47, ofrobots wrote:
> Likewise.

Done.

[mattloring, 2017-01-12 19:45:49]

Description was changed from

==========
FFI Compiler based on code stub assembler

BUG=
==========

to

==========
FFI Compiler outline based on code stub assembler. We are looking to land this
frame to allow specific type translation implementations to proceed in parallel.

BUG=
==========

[mattloring, 2017-01-12 19:45:49]

mattloring@google.com changed reviewers:
+ bmeurer@chromium.org

[Benedikt Meurer, 2017-01-16 11:57:04]

bmeurer@chromium.org changed reviewers:
+ mstarzinger@chromium.org

[Benedikt Meurer, 2017-01-16 11:57:04]

bmeurer@chromium.org changed required reviewers:
+ mstarzinger@chromium.org

[Benedikt Meurer, 2017-01-16 11:57:05]

The ffi-compiler shouldn't be inside the compiler directory, but it should
ideally be in a separate ffi folder (similar trick to wasm for the linkage).

Otherwise LGTM from my side.

[Michael Starzinger, 2017-01-16 12:03:17]

Yes, +1 on not having the FFI-compiler be inside the "compile" directory.

[Michael Starzinger, 2017-01-16 12:10:12]

https://codereview.chromium.org/2607993003/diff/100001/src/compiler/code-asse...
File src/compiler/code-assembler.h (right):

https://codereview.chromium.org/2607993003/diff/100001/src/compiler/code-asse...
src/compiler/code-assembler.h:367: Node* CallCFunctionN(CallDescriptor*
descriptor, int input_count,
The {compiler::CallDescriptor} struct is not available outside of the compiler
component. The {CodeAssembler} however represents an API for other components,
so we shouldn't make it part of the interface.

[Michael Starzinger, 2017-01-16 12:32:01]

https://codereview.chromium.org/2607993003/diff/100001/src/compiler/ffi-compi...
File src/compiler/ffi-compiler.cc (right):

https://codereview.chromium.org/2607993003/diff/100001/src/compiler/ffi-compi...
src/compiler/ffi-compiler.cc:83:
Linkage::GetJSCallContextParamIndex(params_with_recv));
Once the FFI-compiler moves to outside of the "compiler" directory, there won't
be a clean way to compute this parameter index. The {FastAccessorAssembler}
already ran into a similar problem. There is a TODO in that regard there:

  // TODO(vogelheim): There is currently no clean way to retrieve the context
  //     parameter for a stub and the implementation details are hidden in
  //     compiler/*. The context_paramter is computed as:
  //       Linkage::GetJSCallContextParamIndex(descriptor->JSParameterCount())

I would be fine with adding a {CodeAssembler::GetJSContextParameter} helper that
just returns the appropriate parameter node. This helper should DCHECK whether
we are generating code with a JS-linkage and then base the index computation on
the parameter-count stored in that linkage.

[mattloring, 2017-01-17 20:01:18]

On 2017/01/16 12:32:01, Michael Starzinger wrote:
>
https://codereview.chromium.org/2607993003/diff/100001/src/compiler/ffi-compi...
> File src/compiler/ffi-compiler.cc (right):
> 
>
https://codereview.chromium.org/2607993003/diff/100001/src/compiler/ffi-compi...
> src/compiler/ffi-compiler.cc:83:
> Linkage::GetJSCallContextParamIndex(params_with_recv));
> Once the FFI-compiler moves to outside of the "compiler" directory, there
won't
> be a clean way to compute this parameter index. The {FastAccessorAssembler}
> already ran into a similar problem. There is a TODO in that regard there:
> 
>   // TODO(vogelheim): There is currently no clean way to retrieve the context
>   //     parameter for a stub and the implementation details are hidden in
>   //     compiler/*. The context_paramter is computed as:
>   //       Linkage::GetJSCallContextParamIndex(descriptor->JSParameterCount())
> 
> I would be fine with adding a {CodeAssembler::GetJSContextParameter} helper
that
> just returns the appropriate parameter node. This helper should DCHECK whether
> we are generating code with a JS-linkage and then base the index computation
on
> the parameter-count stored in that linkage.

I've added this helper function.

[mattloring, 2017-01-17 20:01:37]

On 2017/01/16 12:10:12, Michael Starzinger wrote:
>
https://codereview.chromium.org/2607993003/diff/100001/src/compiler/code-asse...
> File src/compiler/code-assembler.h (right):
> 
>
https://codereview.chromium.org/2607993003/diff/100001/src/compiler/code-asse...
> src/compiler/code-assembler.h:367: Node* CallCFunctionN(CallDescriptor*
> descriptor, int input_count,
> The {compiler::CallDescriptor} struct is not available outside of the compiler
> component. The {CodeAssembler} however represents an API for other components,
> so we shouldn't make it part of the interface.

I've updated the interface to depend on a Signature instead.

[mattloring, 2017-01-17 20:01:49]

On 2017/01/16 12:03:17, Michael Starzinger wrote:
> Yes, +1 on not having the FFI-compiler be inside the "compile" directory.

Moved.

[mattloring, 2017-01-17 20:02:37]

The ffi-compiler now lives in its own directory (src/ffi) and does not depend on
(src/compiler).

[mattloring, 2017-01-17 21:06:04]

The CQ bit was checked by mattloring@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 21:06:09]

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-17 21:07:41]

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

[commit-bot: I haz the power, 2017-01-17 21:07:41]

Dry run: Try jobs failed on following builders:
  v8_android_arm_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_android_arm_compile_rel/...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/19354)
  v8_linux_arm64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm64_rel_ng/build...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19414)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_linux_nodcheck_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel_ng/bu...)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/19324)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/32608)
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21063)

[mattloring, 2017-01-17 21:14:05]

The CQ bit was checked by mattloring@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-17 21:14:09]

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-17 21:44:22]

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

[commit-bot: I haz the power, 2017-01-17 21:44:23]

Dry run: This issue passed the CQ dry run.

[Michael Starzinger, 2017-01-18 10:47:23]

mstarzinger@chromium.org changed reviewers:
+ ishell@chromium.org

[Michael Starzinger, 2017-01-18 10:47:24]

LGTM on the plumbing outside the "ffi" directory, just skimmed the "ffi"
internals.

If you want additional advice on how to structure to FFI-compiler/assembler,
then you could reach out to Igor. He has done a lot of work on our other
assemblers (that inherit from the CodeAssembler) and might be able to help with
some best practices or tricks. I have CCed him on this CL just in case.

https://codereview.chromium.org/2607993003/diff/160001/src/compiler/code-asse...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2607993003/diff/160001/src/compiler/code-asse...
src/compiler/code-assembler.cc:254: DCHECK(desc->kind() ==
CallDescriptor::kCallJSFunction);
nit: DCHECK(desc->IsJSFunctionCall())

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.cc
File src/ffi/ffi-compiler.cc (right):

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.c...
src/ffi/ffi-compiler.cc:1: // Copyright 2016 the V8 project authors. All rights
reserved.
nit: 2017

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.h
File src/ffi/ffi-compiler.h (right):

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.h...
src/ffi/ffi-compiler.h:1: // Copyright 2016 the V8 project authors. All rights
reserved.
nit: 2017

https://codereview.chromium.org/2607993003/diff/160001/test/cctest/ffi/test-f...
File test/cctest/ffi/test-ffi.cc (right):

https://codereview.chromium.org/2607993003/diff/160001/test/cctest/ffi/test-f...
test/cctest/ffi/test-ffi.cc:1: // Copyright 2016 the V8 project authors. All
rights reserved.
nit: 2017

[Igor Sheludko, 2017-01-18 15:20:22]

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.cc
File src/ffi/ffi-compiler.cc (right):

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.c...
src/ffi/ffi-compiler.cc:42: Node* ToJS(CodeStubAssembler* assembler, Node* node,
Node* context,
I'd suggest to define a FFIAssembler (or similar) class inheriting from
CodeStubAssembler with ToJS(), FromJS() and GenerateJSToNativeWrapper()
methods...

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.c...
src/ffi/ffi-compiler.cc:73: CodeStubAssembler assembler(&state);
... create it here, call
  assembler.GenerateJSToNativeWrapper();

and put all the code between lines 75-93 to GenerateJSToNativeWrapper.

See PromiseBuiltinsAssembler for example.

[mattloring, 2017-01-18 17:48:19]

https://codereview.chromium.org/2607993003/diff/160001/src/compiler/code-asse...
File src/compiler/code-assembler.cc (right):

https://codereview.chromium.org/2607993003/diff/160001/src/compiler/code-asse...
src/compiler/code-assembler.cc:254: DCHECK(desc->kind() ==
CallDescriptor::kCallJSFunction);
On 2017/01/18 10:47:23, Michael Starzinger wrote:
> nit: DCHECK(desc->IsJSFunctionCall())

Done.

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.cc
File src/ffi/ffi-compiler.cc (right):

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.c...
src/ffi/ffi-compiler.cc:1: // Copyright 2016 the V8 project authors. All rights
reserved.
On 2017/01/18 10:47:23, Michael Starzinger wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.c...
src/ffi/ffi-compiler.cc:42: Node* ToJS(CodeStubAssembler* assembler, Node* node,
Node* context,
On 2017/01/18 15:20:22, Igor Sheludko wrote:
> I'd suggest to define a FFIAssembler (or similar) class inheriting from
> CodeStubAssembler with ToJS(), FromJS() and GenerateJSToNativeWrapper()
> methods...

Done.

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.c...
src/ffi/ffi-compiler.cc:73: CodeStubAssembler assembler(&state);
On 2017/01/18 15:20:22, Igor Sheludko wrote:
> ... create it here, call
>   assembler.GenerateJSToNativeWrapper();
> 
> and put all the code between lines 75-93 to GenerateJSToNativeWrapper.
> 
> See PromiseBuiltinsAssembler for example.

Done.

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.h
File src/ffi/ffi-compiler.h (right):

https://codereview.chromium.org/2607993003/diff/160001/src/ffi/ffi-compiler.h...
src/ffi/ffi-compiler.h:1: // Copyright 2016 the V8 project authors. All rights
reserved.
On 2017/01/18 10:47:24, Michael Starzinger wrote:
> nit: 2017

Done.

https://codereview.chromium.org/2607993003/diff/160001/test/cctest/ffi/test-f...
File test/cctest/ffi/test-ffi.cc (right):

https://codereview.chromium.org/2607993003/diff/160001/test/cctest/ffi/test-f...
test/cctest/ffi/test-ffi.cc:1: // Copyright 2016 the V8 project authors. All
rights reserved.
On 2017/01/18 10:47:24, Michael Starzinger wrote:
> nit: 2017

Done.

[mattloring, 2017-01-18 17:55:09]

The CQ bit was checked by mattloring@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 17:55:12]

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-18 17:56:42]

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

[commit-bot: I haz the power, 2017-01-18 17:56:43]

Dry run: Try jobs failed on following builders:
  v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...)
  v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/19514)
  v8_linux_gcc_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_gcc_compile_rel/bu...)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/32742)

[Igor Sheludko, 2017-01-18 18:18:14]

nits:

https://codereview.chromium.org/2607993003/diff/180001/src/ffi/ffi-compiler.cc
File src/ffi/ffi-compiler.cc (right):

https://codereview.chromium.org/2607993003/diff/180001/src/ffi/ffi-compiler.c...
src/ffi/ffi-compiler.cc:70: return GenerateCode(state());
I'd move the GenerateCode() call to CompileJSToNativeWrapper() to be consistent
with the other assemblers.

https://codereview.chromium.org/2607993003/diff/180001/src/ffi/ffi-compiler.h
File src/ffi/ffi-compiler.h (right):

https://codereview.chromium.org/2607993003/diff/180001/src/ffi/ffi-compiler.h...
src/ffi/ffi-compiler.h:28: class FFIAssembler : public CodeStubAssembler {
Since nobody uses it outside the .cc file, it could be defined there.

[mattloring, 2017-01-18 18:19:14]

The CQ bit was checked by mattloring@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 18:19:18]

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

[mattloring, 2017-01-18 18:26:24]

https://codereview.chromium.org/2607993003/diff/180001/src/ffi/ffi-compiler.cc
File src/ffi/ffi-compiler.cc (right):

https://codereview.chromium.org/2607993003/diff/180001/src/ffi/ffi-compiler.c...
src/ffi/ffi-compiler.cc:70: return GenerateCode(state());
On 2017/01/18 18:18:14, Igor Sheludko wrote:
> I'd move the GenerateCode() call to CompileJSToNativeWrapper() to be
consistent
> with the other assemblers.

Done.

https://codereview.chromium.org/2607993003/diff/180001/src/ffi/ffi-compiler.h
File src/ffi/ffi-compiler.h (right):

https://codereview.chromium.org/2607993003/diff/180001/src/ffi/ffi-compiler.h...
src/ffi/ffi-compiler.h:28: class FFIAssembler : public CodeStubAssembler {
On 2017/01/18 18:18:14, Igor Sheludko wrote:
> Since nobody uses it outside the .cc file, it could be defined there. 

Done.

[Igor Sheludko, 2017-01-18 18:28:34]

lgtm. thanks!

[mattloring, 2017-01-18 18:35:23]

Description was changed from

==========
FFI Compiler outline based on code stub assembler. We are looking to land this
frame to allow specific type translation implementations to proceed in parallel.

BUG=
==========

to

==========
FFI Compiler outline based on code stub assembler. We are looking to land this
frame to allow specific type translation implementations to proceed in parallel.

BUG=v8:4456
==========

[ofrobots, 2017-01-18 18:36:16]

On 2017/01/18 18:28:34, Igor Sheludko wrote:
> lgtm. thanks!

LGTM

[mattloring, 2017-01-18 18:37:01]

The CQ bit was checked by mattloring@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 18:37:14]

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-18 19:09:11]

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

[commit-bot: I haz the power, 2017-01-18 19:09:12]

Dry run: This issue passed the CQ dry run.

[mattloring, 2017-01-18 19:11:21]

The CQ bit was checked by mattloring@google.com

[mattloring, 2017-01-18 19:11:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/2607993003/#ps220001
(title: "Move FFIAssembler from .h to .cc")

[commit-bot: I haz the power, 2017-01-18 19:11:32]

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-18 19:13:51]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1484766681469240,
"parent_rev": "970d90767dce7fa13e0f0aa5b4eaea83af710e65", "commit_rev":
"0ecc6b060045d617ac31d94816054d2a3cca7855"}

[commit-bot: I haz the power, 2017-01-18 19:13:56]

Description was changed from

==========
FFI Compiler outline based on code stub assembler. We are looking to land this
frame to allow specific type translation implementations to proceed in parallel.

BUG=v8:4456
==========

to

==========
FFI Compiler outline based on code stub assembler. We are looking to land this
frame to allow specific type translation implementations to proceed in parallel.

BUG=v8:4456

Review-Url: https://codereview.chromium.org/2607993003
Cr-Commit-Position: refs/heads/master@{#42475}
Committed:
https://chromium.googlesource.com/v8/v8/+/0ecc6b060045d617ac31d94816054d2a3cc...
==========

[commit-bot: I haz the power, 2017-01-18 19:13:58]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/v8/v8/+/0ecc6b060045d617ac31d94816054d2a3cc...