[Interpreter] Implements calls through CallICStub in the interpreter.

[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub.

MIPS port contributed by balazs.kilvady.

BUG=v8:4280, v8:4680
LOG=N

Committed: https://crrev.com/20362a2214c11a0f2ea5141b6a79e09458939cec
Cr-Commit-Position: refs/heads/master@{#34244}

[mythria, 2016-02-12 13:30:32]

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

[commit-bot: I haz the power, 2016-02-12 13:30:41]

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

[mythria, 2016-02-12 13:35:10]

mythria@chromium.org changed reviewers:
+ mvstanton@chromium.org, rmcilroy@chromium.org

[mythria, 2016-02-12 13:35:11]

I implemented a new buitin to support calls through CallICStub in the
interpreter. I implemented it for x86 and ia32 platforms. I will port it to
other architectures after the initial reviews. 

PTAL,

Thanks,
Mythri.

[commit-bot: I haz the power, 2016-02-12 13:56:13]

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

[commit-bot: I haz the power, 2016-02-12 13:56:13]

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

[mythria, 2016-02-12 14:16:13]

Description was changed from

==========
[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub. Lookupslot and eval calls do not go through CallICStub.

BUG=v8:4280, v8:4680
==========

to

==========
[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub. Lookupslot and eval calls do not go through CallICStub.

BUG=v8:4280, v8:4680
==========

[mythria, 2016-02-12 14:16:47]

+v8-ppc-ports
+v8-mips-ports

[rmcilroy, 2016-02-12 14:21:04]

Looks good overrall, couple of suggestions (although Michael knows this area
better than me so please wait for him to weigh in before porting).

https://codereview.chromium.org/1688283003/diff/1/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/1688283003/diff/1/src/builtins.h#newcode211
src/builtins.h:211: V(InterpreterPushArgsAndCallIC, BUILTIN, UNINITIALIZED,
kNoExtraICState)     \
nit - same order as below (CallIC, Call, Construct)

https://codereview.chromium.org/1688283003/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/1688283003/diff/1/src/code-stubs.h#newcode955
src/code-stubs.h:955: int arg_count() const { return state().argc(); }
Add a DCHECK(!argc_in_register())

https://codereview.chromium.org/1688283003/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1688283003/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1041: void
BytecodeGraphBuilder::BuildCall(bool has_feedback_slot) {
Not keen on this, I'd prefer it just to be two seperate functions (BuildCall and
BuildCallWithFeedbackSlot)

https://codereview.chromium.org/1688283003/diff/1/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/1688283003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:726: __ neg(eax);
Could you try and use a virtual register instead, e.g.:
(ExternalReference::virtual_handler_register(masm->isolate()).

https://codereview.chromium.org/1688283003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:733: __ add(ecx, ebx);
Could you pull this out to a helper function (to share with
InterpreterPushArgsAndCall)

https://codereview.chromium.org/1688283003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:737: // Call via the CallIC stub.
nit - move this comment above CallIcState call_ic...

https://codereview.chromium.org/1688283003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:744: __ CallStub(&stub);
Add a TODO that we should replace this with a TailCall once we update the code
to be able to find the target IC for jump instructions.

https://codereview.chromium.org/1688283003/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1688283003/diff/1/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:1900: // ebx - vector
Add a comment here that eax contains arg_count if !argc_in_register()

https://codereview.chromium.org/1688283003/diff/1/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:1972: }
Could we just pull this up to be below "argc = arg_count()" (and move that line
into this if) and get rid of the __ Set(eax, argc); on line 2004 as well? That
would make the two versions of the stub much more similar (i.e., both would need
to retain eax throughout the stub)

https://codereview.chromium.org/1688283003/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1688283003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.h:170: // Call through CallICStub to get
Typefeedback.
nit - newline above comment, and add same details as Call's comment header (and
move CallIC to be first)

https://codereview.chromium.org/1688283003/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1688283003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2289: call_type == Call::LOOKUP_SLOT_CALL)
{
Would this be the same if you did:
if (expr->CallFeedbackICSlot().IsInvalid()) {
...


That would seem safer to me.

https://codereview.chromium.org/1688283003/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/1688283003/diff/1/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:1755: // rcx - allocation site (loaded from
vector[slot]).
Same comment as ia32.

https://codereview.chromium.org/1688283003/diff/1/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:1771: if (!argc_in_register()) {
nit - Swap branches around (avoid ! in condition)

https://codereview.chromium.org/1688283003/diff/1/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:1823: if (!argc_in_register()) {
Same comment as ia32

[mythria, 2016-02-12 16:10:03]

Description was changed from

==========
[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub. Lookupslot and eval calls do not go through CallICStub.

BUG=v8:4280, v8:4680
==========

to

==========
[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub. 

BUG=v8:4280, v8:4680
==========

[mythria, 2016-02-12 16:10:25]

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

[commit-bot: I haz the power, 2016-02-12 16:10:36]

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

[commit-bot: I haz the power, 2016-02-12 16:12:57]

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

[commit-bot: I haz the power, 2016-02-12 16:12:58]

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

[mythria, 2016-02-12 16:29:01]

Description was changed from

==========
[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub. 

BUG=v8:4280, v8:4680
==========

to

==========
[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub. 

BUG=v8:4280, v8:4680
LOG=N
==========

[mvstanton, 2016-02-15 11:13:46]

Hi Mythri, looking good, I have a few comments. If an extra push/pop somewhere
can avoid using that virtual register it would be really nice.
Thanks,
--Mike

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:729: ExternalReference virtual_register =
I'd love it if we don't have to expand the usage of this virtual register, which
was quite a hack just to support the ia32 fast case on store ics. Is there any
other option?

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:733:
Generate_InterpreterComputeLastArgumentAddress(masm);
It would be nice if InterpreterComputeLastArgumentAddress took it's registers as
input. It's like a small helper function, it would be nice to be able to see
what it clobbers just by reading the call site.

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:1921: ArrayConstructorStub stub(masm->isolate(),
arg_count());
Okay, so you are sure ArrayConstructorStub doesn't access EAX when you pass in a
constant arg count?
It would be good to comment that assumption.

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

https://codereview.chromium.org/1688283003/diff/40001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:790: 
Wow, more registers makes things so much nicer :p.

https://codereview.chromium.org/1688283003/diff/40001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/1688283003/diff/40001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:1898: 
Wow, is it just a bug that rax wasn't being saved?

[rmcilroy, 2016-02-15 11:20:50]

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:729: ExternalReference virtual_register =
On 2016/02/15 11:13:45, mvstanton wrote:
> I'd love it if we don't have to expand the usage of this virtual register,
which
> was quite a hack just to support the ia32 fast case on store ics. Is there any
> other option?

Sorry, this was my fault suggesting it as I thought it was cleaner than the
approach of pushing things to a random point on the stack. The messiness with
that approach is that we need to push it higher than the current sp since we are
about to push arguments on the stack - i.e., (in patch set 1):

  721     // Store the feedback vector below where arguments are going to be
pushed.
  722     // This would act like a temporary store without disturbing the stack.
  723     // We push eax + 1 values, so adjust the displacement accordingly.
  724     __ neg(eax);
  725     __ mov(Operand(esp, eax, times_pointer_size, -2 * kPointerSize), ecx);
  726     __ neg(eax);
  ...
  <Push arguments between the previous sp and where we have just pushed feedback
vector>
  ...
  738     // Restore feedback vector to ebx from the stack.
  739     __ mov(ebx, Operand(esp, -kPointerSize));

Happy to go back to that approach if you think it's better.

[mvstanton, 2016-02-15 11:32:04]

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:729: ExternalReference virtual_register =
On 2016/02/15 11:20:50, rmcilroy wrote:
> On 2016/02/15 11:13:45, mvstanton wrote:
> > I'd love it if we don't have to expand the usage of this virtual register,
> which
> > was quite a hack just to support the ia32 fast case on store ics. Is there
any
> > other option?
> 
> Sorry, this was my fault suggesting it as I thought it was cleaner than the
> approach of pushing things to a random point on the stack. The messiness with
> that approach is that we need to push it higher than the current sp since we
are
> about to push arguments on the stack - i.e., (in patch set 1):
> 
>   721     // Store the feedback vector below where arguments are going to be
> pushed.
>   722     // This would act like a temporary store without disturbing the
stack.
>   723     // We push eax + 1 values, so adjust the displacement accordingly.
>   724     __ neg(eax);
>   725     __ mov(Operand(esp, eax, times_pointer_size, -2 * kPointerSize),
ecx);
>   726     __ neg(eax);
>   ...
>   <Push arguments between the previous sp and where we have just pushed
feedback
> vector>
>   ...
>   738     // Restore feedback vector to ebx from the stack.
>   739     __ mov(ebx, Operand(esp, -kPointerSize));
> 
> Happy to go back to that approach if you think it's better.

LOL, I know, I am tempted too..! I think the virtual register will bite us
though, or at least more usage of it will make it harder to remove at some
point, so let's use that somewhat tricky stack code.

[mythria, 2016-02-17 11:02:48]

Thanks for the review. I addressed your comments. I am trying to fix one failing
test and then I will port it to other architectures as well.

Thanks,
Mythri

https://codereview.chromium.org/1688283003/diff/1/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/1688283003/diff/1/src/builtins.h#newcode211
src/builtins.h:211: V(InterpreterPushArgsAndCallIC, BUILTIN, UNINITIALIZED,
kNoExtraICState)     \
On 2016/02/12 14:21:03, rmcilroy wrote:
> nit - same order as below (CallIC, Call, Construct)

Done.

https://codereview.chromium.org/1688283003/diff/1/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/1688283003/diff/1/src/code-stubs.h#newcode955
src/code-stubs.h:955: int arg_count() const { return state().argc(); }
On 2016/02/12 14:21:03, rmcilroy wrote:
> Add a DCHECK(!argc_in_register())

Done.

https://codereview.chromium.org/1688283003/diff/1/src/compiler/bytecode-graph...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1688283003/diff/1/src/compiler/bytecode-graph...
src/compiler/bytecode-graph-builder.cc:1041: void
BytecodeGraphBuilder::BuildCall(bool has_feedback_slot) {
On 2016/02/12 14:21:03, rmcilroy wrote:
> Not keen on this, I'd prefer it just to be two seperate functions (BuildCall
and
> BuildCallWithFeedbackSlot)

Done.

https://codereview.chromium.org/1688283003/diff/1/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/1688283003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:726: __ neg(eax);
On 2016/02/12 14:21:03, rmcilroy wrote:
> Could you try and use a virtual register instead, e.g.:
> (ExternalReference::virtual_handler_register(masm->isolate()).

Done.

https://codereview.chromium.org/1688283003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:733: __ add(ecx, ebx);
On 2016/02/12 14:21:03, rmcilroy wrote:
> Could you pull this out to a helper function (to share with
> InterpreterPushArgsAndCall)

Done.

https://codereview.chromium.org/1688283003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:737: // Call via the CallIC stub.
On 2016/02/12 14:21:03, rmcilroy wrote:
> nit - move this comment above CallIcState call_ic...

Done.

https://codereview.chromium.org/1688283003/diff/1/src/ia32/builtins-ia32.cc#n...
src/ia32/builtins-ia32.cc:744: __ CallStub(&stub);
On 2016/02/12 14:21:03, rmcilroy wrote:
> Add a TODO that we should replace this with a TailCall once we update the code
> to be able to find the target IC for jump instructions.

Done.

https://codereview.chromium.org/1688283003/diff/1/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1688283003/diff/1/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:1900: // ebx - vector
On 2016/02/12 14:21:03, rmcilroy wrote:
> Add a comment here that eax contains arg_count if !argc_in_register()

Done.

https://codereview.chromium.org/1688283003/diff/1/src/ia32/code-stubs-ia32.cc...
src/ia32/code-stubs-ia32.cc:1972: }
On 2016/02/12 14:21:03, rmcilroy wrote:
> Could we just pull this up to be below "argc = arg_count()" (and move that
line
> into this if) and get rid of the __ Set(eax, argc); on line 2004 as well? That
> would make the two versions of the stub much more similar (i.e., both would
need
> to retain eax throughout the stub)

Done.

https://codereview.chromium.org/1688283003/diff/1/src/interpreter/bytecode-ar...
File src/interpreter/bytecode-array-builder.h (right):

https://codereview.chromium.org/1688283003/diff/1/src/interpreter/bytecode-ar...
src/interpreter/bytecode-array-builder.h:170: // Call through CallICStub to get
Typefeedback.
On 2016/02/12 14:21:03, rmcilroy wrote:
> nit - newline above comment, and add same details as Call's comment header
(and
> move CallIC to be first)

Done.

https://codereview.chromium.org/1688283003/diff/1/src/interpreter/bytecode-ge...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/1688283003/diff/1/src/interpreter/bytecode-ge...
src/interpreter/bytecode-generator.cc:2289: call_type == Call::LOOKUP_SLOT_CALL)
{
On 2016/02/12 14:21:03, rmcilroy wrote:
> Would this be the same if you did:
> if (expr->CallFeedbackICSlot().IsInvalid()) {
> ...
> 
> 
> That would seem safer to me.

Done. Yes, indeed. I missed it but Lookup_slot_calls go through callIC too. I
verified it in full-codegen.

https://codereview.chromium.org/1688283003/diff/1/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/1688283003/diff/1/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:1755: // rcx - allocation site (loaded from
vector[slot]).
On 2016/02/12 14:21:03, rmcilroy wrote:
> Same comment as ia32.

Done.

https://codereview.chromium.org/1688283003/diff/1/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:1771: if (!argc_in_register()) {
On 2016/02/12 14:21:03, rmcilroy wrote:
> nit - Swap branches around (avoid ! in condition)

Done.

https://codereview.chromium.org/1688283003/diff/1/src/x64/code-stubs-x64.cc#n...
src/x64/code-stubs-x64.cc:1823: if (!argc_in_register()) {
On 2016/02/12 14:21:03, rmcilroy wrote:
> Same comment as ia32

Done.

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:729: ExternalReference virtual_register =
On 2016/02/15 11:32:04, mvstanton wrote:
> On 2016/02/15 11:20:50, rmcilroy wrote:
> > On 2016/02/15 11:13:45, mvstanton wrote:
> > > I'd love it if we don't have to expand the usage of this virtual register,
> > which
> > > was quite a hack just to support the ia32 fast case on store ics. Is there
> any
> > > other option?
> > 
> > Sorry, this was my fault suggesting it as I thought it was cleaner than the
> > approach of pushing things to a random point on the stack. The messiness
with
> > that approach is that we need to push it higher than the current sp since we
> are
> > about to push arguments on the stack - i.e., (in patch set 1):
> > 
> >   721     // Store the feedback vector below where arguments are going to be
> > pushed.
> >   722     // This would act like a temporary store without disturbing the
> stack.
> >   723     // We push eax + 1 values, so adjust the displacement accordingly.
> >   724     __ neg(eax);
> >   725     __ mov(Operand(esp, eax, times_pointer_size, -2 * kPointerSize),
> ecx);
> >   726     __ neg(eax);
> >   ...
> >   <Push arguments between the previous sp and where we have just pushed
> feedback
> > vector>
> >   ...
> >   738     // Restore feedback vector to ebx from the stack.
> >   739     __ mov(ebx, Operand(esp, -kPointerSize));
> > 
> > Happy to go back to that approach if you think it's better.
> 
> LOL, I know, I am tempted too..! I think the virtual register will bite us
> though, or at least more usage of it will make it harder to remove at some
> point, so let's use that somewhat tricky stack code.

Done.

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/builtins-ia32....
src/ia32/builtins-ia32.cc:733:
Generate_InterpreterComputeLastArgumentAddress(masm);
On 2016/02/15 11:13:45, mvstanton wrote:
> It would be nice if InterpreterComputeLastArgumentAddress took it's registers
as
> input. It's like a small helper function, it would be nice to be able to see
> what it clobbers just by reading the call site.

We just clobber ecx, though we use eax and ebx as well. Would it be better if I
pass eax and ebx?

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/code-stubs-ia3...
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/1688283003/diff/40001/src/ia32/code-stubs-ia3...
src/ia32/code-stubs-ia32.cc:1921: ArrayConstructorStub stub(masm->isolate(),
arg_count());
On 2016/02/15 11:13:45, mvstanton wrote:
> Okay, so you are sure ArrayConstructorStub doesn't access EAX when you pass in
a
> constant arg count?
> It would be good to comment that assumption.

Actually, it expects in eax if arg_count() >= 2. I added a comment to make it
clear. Done.

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

https://codereview.chromium.org/1688283003/diff/40001/src/x64/builtins-x64.cc...
src/x64/builtins-x64.cc:790: 
On 2016/02/15 11:13:45, mvstanton wrote:
> Wow, more registers makes things so much nicer :p.

Yes :).

https://codereview.chromium.org/1688283003/diff/40001/src/x64/code-stubs-x64.cc
File src/x64/code-stubs-x64.cc (right):

https://codereview.chromium.org/1688283003/diff/40001/src/x64/code-stubs-x64....
src/x64/code-stubs-x64.cc:1898: 
On 2016/02/15 11:13:45, mvstanton wrote:
> Wow, is it just a bug that rax wasn't being saved?

Yes :(

[mythria, 2016-02-17 14:10:01]

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

[commit-bot: I haz the power, 2016-02-17 14:10:07]

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

[commit-bot: I haz the power, 2016-02-17 14:10:59]

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

[commit-bot: I haz the power, 2016-02-17 14:11:00]

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

[balazs.kilvady, 2016-02-17 16:19:26]

On 2016/02/12 14:16:47, mythria wrote:
> +v8-ppc-ports
> +v8-mips-ports

We are working on the MIPS port.

[mvstanton, 2016-02-18 00:55:48]

lgtm

[mythria, 2016-02-18 10:27:25]

On 2016/02/17 16:19:26, balazs.kilvady wrote:
> On 2016/02/12 14:16:47, mythria wrote:
> > +v8-ppc-ports
> > +v8-mips-ports
> 
> We are working on the MIPS port.

Thanks.

[mythria, 2016-02-18 11:21:59]

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

[commit-bot: I haz the power, 2016-02-18 11:22:04]

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

[commit-bot: I haz the power, 2016-02-18 11:27:07]

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

[commit-bot: I haz the power, 2016-02-18 11:27:08]

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...)

[mythria, 2016-02-18 14:46:47]

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

[commit-bot: I haz the power, 2016-02-18 14:46:59]

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

[commit-bot: I haz the power, 2016-02-18 14:57:01]

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

[commit-bot: I haz the power, 2016-02-18 14:57:02]

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...)

[balazs.kilvady, 2016-02-18 19:27:03]

On 2016/02/18 10:27:25, mythria wrote:
> On 2016/02/17 16:19:26, balazs.kilvady wrote:
> > On 2016/02/12 14:16:47, mythria wrote:
> > > +v8-ppc-ports
> > > +v8-mips-ports
> > 
> > We are working on the MIPS port.
> 
> Thanks.

Please find the MIPS port at: https://codereview.chromium.org/1706263002/

[mythria, 2016-02-22 11:58:31]

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

[commit-bot: I haz the power, 2016-02-22 11:58:42]

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

[commit-bot: I haz the power, 2016-02-22 12:04:02]

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

[commit-bot: I haz the power, 2016-02-22 12:04:04]

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...)

[mythria, 2016-02-22 12:31:15]

Description was changed from

==========
[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub. 

BUG=v8:4280, v8:4680
LOG=N
==========

to

==========
[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub. 
	
MIPS port contributed by balazs.kilvady.

BUG=v8:4280, v8:4680
LOG=N
==========

[mythria, 2016-02-22 12:31:41]

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

[commit-bot: I haz the power, 2016-02-22 12:31:55]

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

[rmcilroy, 2016-02-22 12:53:23]

Great stuff, lgtm, thanks.

https://codereview.chromium.org/1688283003/diff/210001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1688283003/diff/210001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:1007: }
nit - move BuildCall(TailCallMode tail_call_mode) down so that it is immediately
above  VisitCall()

https://codereview.chromium.org/1688283003/diff/210001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1688283003/diff/210001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:92: // located at |first_arg| with
feedback slot id |slot_id|.
Add comment about type feedback vector

https://codereview.chromium.org/1688283003/diff/210001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1688283003/diff/210001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:957: tail_call_mode)
You are doing Call and CallIC, but only checking the return value of CallIC. 
Could you make these different tests please (and the same for the cases below).

[commit-bot: I haz the power, 2016-02-22 12:58:41]

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

[commit-bot: I haz the power, 2016-02-22 12:58:42]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-02-22 14:51:32]

Thanks for your reviews. The following are the changes since patchset 6:
1.I added support for tail calls after rebasing. 
2.Fixed a bug in ia32. I was storing the feedback vector on the stack
temporarily instead of pushing it. This means the value is store above esp. If
there is an interrupt/signal, the stored value gets corrupted since the same
stack is used by the interrupt/signal handlers. Now I push into the stack
instead.  
3.I patched Mips port contributed by balazs.kilvady. 
4.Added tests to test-bytecode-generator.
5.Also addressed comments from Ross. 

Thanks and Regards,
Mythri

https://codereview.chromium.org/1688283003/diff/210001/src/compiler/bytecode-...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/1688283003/diff/210001/src/compiler/bytecode-...
src/compiler/bytecode-graph-builder.cc:1007: }
On 2016/02/22 12:53:23, rmcilroy wrote:
> nit - move BuildCall(TailCallMode tail_call_mode) down so that it is
immediately
> above  VisitCall()

Done.

https://codereview.chromium.org/1688283003/diff/210001/src/interpreter/interp...
File src/interpreter/interpreter-assembler.h (right):

https://codereview.chromium.org/1688283003/diff/210001/src/interpreter/interp...
src/interpreter/interpreter-assembler.h:92: // located at |first_arg| with
feedback slot id |slot_id|.
On 2016/02/22 12:53:23, rmcilroy wrote:
> Add comment about type feedback vector

Done.

https://codereview.chromium.org/1688283003/diff/210001/test/cctest/interprete...
File test/cctest/interpreter/test-interpreter.cc (right):

https://codereview.chromium.org/1688283003/diff/210001/test/cctest/interprete...
test/cctest/interpreter/test-interpreter.cc:957: tail_call_mode)
On 2016/02/22 12:53:23, rmcilroy wrote:
> You are doing Call and CallIC, but only checking the return value of CallIC. 
> Could you make these different tests please (and the same for the cases
below).

I am sorry, I thought I will revisit this when rebasing and then forgot. Thanks
for pointing this. Done.

[mythria, 2016-02-22 14:51:58]

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

[commit-bot: I haz the power, 2016-02-22 14:52:09]

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

[commit-bot: I haz the power, 2016-02-22 15:07:07]

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

[commit-bot: I haz the power, 2016-02-22 15:07:08]

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

[mythria, 2016-02-22 15:35:10]

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

[commit-bot: I haz the power, 2016-02-22 15:35:20]

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

[commit-bot: I haz the power, 2016-02-22 15:56:10]

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

[commit-bot: I haz the power, 2016-02-22 15:56:10]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-02-23 09:23:21]

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

[commit-bot: I haz the power, 2016-02-23 09:23:28]

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

[commit-bot: I haz the power, 2016-02-23 09:24:47]

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

[commit-bot: I haz the power, 2016-02-23 09:24:49]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-02-23 09:29:40]

The CQ bit was checked by mythria@chromium.org

[mythria, 2016-02-23 09:29:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/1688283003/#ps250001
(title: "Fixed an error in test-interpreter.")

[commit-bot: I haz the power, 2016-02-23 09:29:46]

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

[commit-bot: I haz the power, 2016-02-23 09:33:00]

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

[commit-bot: I haz the power, 2016-02-23 09:33:02]

Try jobs failed on following builders:
  v8_presubmit on tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/11353)

[mythria, 2016-02-23 09:48:17]

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

[mythria, 2016-02-23 09:48:17]

Hi Michi,

Could you please take a look at compiler changes. This cl adds new bytecodes to
support calls through callIC. the original call (without type feedback) still
exists because of possibly eval calls.

Thanks and Regards,
Mythri.

[Michael Starzinger, 2016-02-24 10:21:46]

LGTM, sorry for the delay, this one slipped through the cracks.

[mythria, 2016-02-24 10:27:23]

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

[commit-bot: I haz the power, 2016-02-24 10:27:42]

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

[commit-bot: I haz the power, 2016-02-24 10:52:33]

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

[commit-bot: I haz the power, 2016-02-24 10:52:35]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-02-24 10:52:51]

The CQ bit was checked by mythria@chromium.org

[mythria, 2016-02-24 10:52:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvstanton@chromium.org, rmcilroy@chromium.org, mstarzinger@chromium.org
Link to the patchset: https://codereview.chromium.org/1688283003/#ps270001
(title: "removes an unused label declaration.")

[commit-bot: I haz the power, 2016-02-24 10:53:06]

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

[commit-bot: I haz the power, 2016-02-24 11:00:55]

Description was changed from

==========
[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub. 
	
MIPS port contributed by balazs.kilvady.

BUG=v8:4280, v8:4680
LOG=N
==========

to

==========
[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub. 
	
MIPS port contributed by balazs.kilvady.

BUG=v8:4280, v8:4680
LOG=N
==========

[commit-bot: I haz the power, 2016-02-24 11:00:57]

Committed patchset #15 (id:270001)

[commit-bot: I haz the power, 2016-02-24 11:01:34]

Description was changed from

==========
[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub. 
	
MIPS port contributed by balazs.kilvady.

BUG=v8:4280, v8:4680
LOG=N
==========

to

==========
[Interpreter] Implements calls through CallICStub in the interpreter.

Calls are implemented through CallICStub to collect type feedback. Adds
a new builtin called InterpreterPushArgsAndCallIC that pushes the
arguments onto stack and calls CallICStub.

Also adds two new bytecodes CallIC and CallICWide to indicate calls have to
go through CallICStub.

MIPS port contributed by balazs.kilvady.

BUG=v8:4280, v8:4680
LOG=N

Committed: https://crrev.com/20362a2214c11a0f2ea5141b6a79e09458939cec
Cr-Commit-Position: refs/heads/master@{#34244}
==========

[commit-bot: I haz the power, 2016-02-24 11:01:36]

Patchset 15 (id:??) landed as
https://crrev.com/20362a2214c11a0f2ea5141b6a79e09458939cec
Cr-Commit-Position: refs/heads/master@{#34244}

[mythria, 2016-02-24 15:14:40]

A revert of this CL (patchset #15 id:270001) has been created in
https://codereview.chromium.org/1731253003/ by mythria@chromium.org.

The reason for reverting is: It is not a good idea to call CallICStub from the
builtin. It might be sensitive to the frame structure. Constructing a internal
frame might cause problems. It is much better to inline the code  related to the
type feedback vector into the builtin. .