[Interpreter] Add support for JS calls.

[Interpreter] Add support for JS calls.

Adds support for JS calls to the interpreter. In order to support
calls from the interpreter, the PushArgsAndCall builtin is added
which pushes a sequence of arguments onto the stack and calls
builtin::Call.

Adds the Call bytecode.

MIPS port contributed by akos.palfi@imgtec.com in https://codereview.chromium.org/1334873002/

BUG=v8:4280
LOG=N

Committed: https://crrev.com/e7fb233946b990ecbbbd76cc6529f62bd5da64e3
Cr-Commit-Position: refs/heads/master@{#30710}

[rmcilroy, 2015-09-04 22:14:47]

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

[rmcilroy, 2015-09-04 22:14:48]

Hi Benedikt. Here is an in-progress CL which adds support for JS calls to the
interpreter as well as adding support for a CallVarArgs instruction in TF as
discussed offline today. Currently it assumes the callable is a JSFunction and
calls it directly, but I'll try patching in your CallCallableStub CL on Monday
and see if that works. Very happy for any suggestions on how this could be
improved, particularly the TurboFan CallVarArgs bits. PTAL, thanks.

[Benedikt Meurer, 2015-09-07 05:05:13]

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

[Benedikt Meurer, 2015-09-07 05:05:14]

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

[Benedikt Meurer, 2015-09-07 05:05:14]

+mstarzinger
+jarin

[Benedikt Meurer, 2015-09-07 05:14:30]

Just two very early comments, and one question:
You mentioned that it should be possible to write certain bytecode handlers as
native builtins. How about doing that for the call opcode? Because that seems
like a lot of machinery in TurboFan to support this very special opcode.

https://codereview.chromium.org/1323463005/diff/1/src/compiler/x64/code-gener...
File src/compiler/x64/code-generator-x64.cc (right):

https://codereview.chromium.org/1323463005/diff/1/src/compiler/x64/code-gener...
src/compiler/x64/code-generator-x64.cc:1247: __ movp(kScratchRegister,
Operand(args_start, 0));
This sequence is probably not x32 safe. You should just use
Push(Operand(args_start, 0)) instead.

https://codereview.chromium.org/1323463005/diff/1/src/compiler/x64/instructio...
File src/compiler/x64/instruction-selector-x64.cc (right):

https://codereview.chromium.org/1323463005/diff/1/src/compiler/x64/instructio...
src/compiler/x64/instruction-selector-x64.cc:1223: Emit(kX64PushMultiple,
g.NoOutput(), g.UseRegister(args_start),
Mhm, I have mixed feelings with this, but on the other hand I cannot think of a
better solution.

[rmcilroy, 2015-09-07 09:16:46]

On 2015/09/07 05:14:30, Benedikt Meurer wrote:
> Just two very early comments, and one question:
> You mentioned that it should be possible to write certain bytecode handlers as
> native builtins. How about doing that for the call opcode? Because that seems
> like a lot of machinery in TurboFan to support this very special opcode.

[Answering question first, I'll fix the other issues once this is decided]

A lot of the machinery comes from the fact that I made this a separate
CallVarArgs ircode as we dicussed offline on Friday to keep things seperatable.
I could reduce this machinery a lot by just relying on the kHasVarArgs in
CallDescriptor and augmenting the Call instruction to support this. Would this
ease your concerns?

What I mentioned originally was being able to write certain (complex) bytecode
handlers in JS instead of raw-assembly - this wouldn't apply in this case. It
might be possible to do in native builtins, but I think making a completely new
bytecode handler code generation path just for this (fairly fundamental)
bytecode would add even more machinery (although admittedly not in TF).

[rmcilroy, 2015-09-07 09:17:12]

rmcilroy@chromium.org changed reviewers:
+ oth@chromium.org

[Benedikt Meurer, 2015-09-07 16:58:12]

On 2015/09/07 09:16:46, rmcilroy wrote:
> On 2015/09/07 05:14:30, Benedikt Meurer wrote:
> > Just two very early comments, and one question:
> > You mentioned that it should be possible to write certain bytecode handlers
as
> > native builtins. How about doing that for the call opcode? Because that
seems
> > like a lot of machinery in TurboFan to support this very special opcode.
> 
> [Answering question first, I'll fix the other issues once this is decided]
> 
> A lot of the machinery comes from the fact that I made this a separate
> CallVarArgs ircode as we dicussed offline on Friday to keep things
seperatable.
> I could reduce this machinery a lot by just relying on the kHasVarArgs in
> CallDescriptor and augmenting the Call instruction to support this. Would this
> ease your concerns?
> 
> What I mentioned originally was being able to write certain (complex) bytecode
> handlers in JS instead of raw-assembly - this wouldn't apply in this case. It
> might be possible to do in native builtins, but I think making a completely
new
> bytecode handler code generation path just for this (fairly fundamental)
> bytecode would add even more machinery (although admittedly not in TF).

So we talked about this for quite some time today, and the problem we see
with this is that there's no real use of CallVarArgs outside the interpreter.
So we have to support this in TurboFan (which might become more serious
as TurboFan matures) even tho this could be implemented with a few lines
of hand-written native code (basically the PushMultiple plus a Call to the
builtin).

And especially, since you might want to avoid leaving and reentering the
interpreter on calls between bytecode functions, you'll probably need to
do some frame magic in the Call bytecode handler anyway (which TF cannot
support in general). So my feeling here that this is trying to crack a walnut
with a sledgehammer. But I that is just my feeling; I don't want to block the
interpreter, which means this is not a N-O-T L-G-T-M from me.

[rmcilroy, 2015-09-08 12:58:10]

Ok. Doing the whole handler as native code would add a whole lot more complexity
(e.g., ensuring that interpreter are saved and restored properly). Instead, I've
written a small builtin called PushArgsAndCall which does the pushing and then
directly calls builtin::Call. This avoids the need for CallVarArgs. How's this?


> And especially, since you might want to avoid leaving and reentering the
> interpreter on calls between bytecode functions, you'll probably need to
> do some frame magic in the Call bytecode handler anyway (which TF cannot
> support in general). 

I don't think we will want to do this - there is very little code in
builtin::EnterInterpreter which we can avoid doing even if we are already in the
interpreter, so it probably isn't worth special casing interpreter->interpreter
calls.

> So my feeling here that this is trying to crack a walnut
> with a sledgehammer. But I that is just my feeling; I don't want to block the
> interpreter, which means this is not a N-O-T L-G-T-M from me.

I appreciate the feeling, although I wish it had been expressed when we
discussed the general approach on Friday... :).

[rmcilroy, 2015-09-09 23:25:08]

Patchset #3 (id:40001) has been deleted

[rmcilroy, 2015-09-10 08:21:16]

Benedikt: I'd like to port this to the other architectures. Before I do so could
you have a look and let me know if you are happy with the general approach?
Thanks.

[Benedikt Meurer, 2015-09-10 09:51:34]

Builtin looks good modulo comment.

https://codereview.chromium.org/1323463005/diff/60001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1323463005/diff/60001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:1795: __ Call(masm->isolate()->builtins()->Call(),
RelocInfo::CODE_TARGET);
Hm, I think this should be Jump instead of Call+ret

[Benedikt Meurer, 2015-09-10 09:52:08]

And general approach looks good to me too.

[oth, 2015-09-10 14:17:28]

lgtm. Minor spelling nit, but otherwise comprehensive.

https://codereview.chromium.org/1323463005/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1323463005/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:71: // |callable|, the receiver should
be in |receiver| and all subsiquent
nit s/subsiquent/subsequent/

[rmcilroy, 2015-09-10 14:25:23]

Thanks for the reviews, I'll port to ia32, arm and arm64 now.

+v8-mips-ports: Could you port for mips/mips64 please?

https://codereview.chromium.org/1323463005/diff/60001/src/interpreter/bytecod...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1323463005/diff/60001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:71: // |callable|, the receiver should
be in |receiver| and all subsiquent
On 2015/09/10 14:17:27, oth wrote:
> nit s/subsiquent/subsequent/

Done.

https://codereview.chromium.org/1323463005/diff/60001/src/x64/builtins-x64.cc
File src/x64/builtins-x64.cc (right):

https://codereview.chromium.org/1323463005/diff/60001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:1795: __ Call(masm->isolate()->builtins()->Call(),
RelocInfo::CODE_TARGET);
On 2015/09/10 09:51:34, Benedikt Meurer wrote:
> Hm, I think this should be Jump instead of Call+ret

As discussed offline, this requires popping and repushing the return address on
x64. Done.

[rmcilroy, 2015-09-10 14:27:16]

Patchset #4 (id:80001) has been deleted

[rmcilroy, 2015-09-10 17:02:15]

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

[commit-bot: I haz the power, 2015-09-10 17:02:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323463005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323463005/140001

[commit-bot: I haz the power, 2015-09-10 17:08:22]

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

[commit-bot: I haz the power, 2015-09-10 17:08:23]

Dry run: Try jobs failed on following builders:
  v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)

[akos.palfi.imgtec, 2015-09-10 20:14:05]

akos.palfi@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com

[akos.palfi.imgtec, 2015-09-10 20:14:06]

Hi Ross - I've uploaded the mips ports:
https://codereview.chromium.org/1334873002
However, there's a test failure (cctest/test-interpreter/InterpreterCall) that I
haven't investigated deeply, since it fails on all arch's.

[rmcilroy, 2015-09-11 13:36:12]

Benedikt, all the ports are now complete - could you review / stamp please?


On 2015/09/10 20:14:06, akos.palfi.imgtec wrote:
> Hi Ross - I've uploaded the mips ports:
> https://codereview.chromium.org/1334873002

Thanks for the port - now integrated.

> However, there's a test failure (cctest/test-interpreter/InterpreterCall) that
I
> haven't investigated deeply, since it fails on all arch's.

Turns out this was specific to when the callee function was compiled using
turbofan (only happened with flags --always-opt and --turbo), in which case it
would return a HeapNumber instead of a Smi. Fixed by making the test use
SameValue instead of a SMI equality check.

[rmcilroy, 2015-09-11 13:37:15]

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

[commit-bot: I haz the power, 2015-09-11 13:37:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323463005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323463005/180001

[commit-bot: I haz the power, 2015-09-11 14:15:53]

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

[commit-bot: I haz the power, 2015-09-11 14:15:54]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2015-09-14 09:09:39]

LGTM (rubber-stamped)

[rmcilroy, 2015-09-14 09:18:19]

The CQ bit was checked by rmcilroy@chromium.org

[rmcilroy, 2015-09-14 09:18:19]

The patchset sent to the CQ was uploaded after l-g-t-m from oth@chromium.org
Link to the patchset: https://codereview.chromium.org/1323463005/#ps180001
(title: "Add MIPS port contributed by akos.palfi@imgtec.com")

[commit-bot: I haz the power, 2015-09-14 09:18:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323463005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323463005/180001

[commit-bot: I haz the power, 2015-09-14 09:18:28]

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

[commit-bot: I haz the power, 2015-09-14 09:18:29]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[rmcilroy, 2015-09-14 09:39:55]

rmcilroy@chromium.org changed required reviewers:
- jarin@chromium.org, mstarzinger@chromium.org

[rmcilroy, 2015-09-14 09:40:14]

The CQ bit was checked by rmcilroy@chromium.org

[commit-bot: I haz the power, 2015-09-14 09:40:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323463005/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323463005/180001

[commit-bot: I haz the power, 2015-09-14 10:05:24]

Committed patchset #8 (id:180001)

[commit-bot: I haz the power, 2015-09-14 10:05:44]

Patchset 8 (id:??) landed as
https://crrev.com/e7fb233946b990ecbbbd76cc6529f62bd5da64e3
Cr-Commit-Position: refs/heads/master@{#30710}

[torbjorng, 2015-09-17 15:37:55]

torbjorng@chromium.org changed reviewers:
+ torbjorng@chromium.org

[torbjorng, 2015-09-17 15:37:55]

This CL apparently triggers BUG=532969.

[rmcilroy, 2015-09-17 15:49:37]

On 2015/09/17 15:37:55, torbjorng wrote:
> This CL apparently triggers BUG=532969.

Opps, sorry. Fix up for review at https://codereview.chromium.org/1351943002/.