[Interpreter] Collect allocation site feedback in call bytecode handler.

[Interpreter] Collect allocation site feedback in call bytecode handler.

Adds support to collect allocation site feedback for Array function calls
to the call bytecode handler.

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

Committed: https://crrev.com/9a31162d9d3137d09063d6040865655b2e386384
Cr-Commit-Position: refs/heads/master@{#39283}

[mythria, 2016-09-02 13:55:02]

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

[commit-bot: I haz the power, 2016-09-02 13: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, 2016-09-02 14:21:58]

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

[commit-bot: I haz the power, 2016-09-02 14:21:59]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-09-02 14:33:45]

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

[mythria, 2016-09-02 14:33:46]

Benedikit, Ross,

I updated call bytecode handler to collect allocation site feedback. We still
don't use it when creating the array because I still call it using CallFunction
builtin which does not use the feedback. To use the feedback, we can go through
ArrayConstructorStub or the runtime. It is not difficult to implement it. I am
not sure if we need it. Please take a look and let me know your suggestions.

Thanks,
Mythri

https://codereview.chromium.org/2307903002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2307903002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:566: context, arg_count, first_arg,
function);
We do not pass allocation site feedback here, so the new array that is created
does not use this information. If I understand correctly, we still collect
feedback and can use it in turbofan. I am not sure if this is sufficient for
now. If this implementation is not Ok, then I can change it to call the
ArrayConstructorStub. We just need an additional builtin to push the arguments
before we call the stub.

[Benedikt Meurer, 2016-09-05 04:11:20]

https://codereview.chromium.org/2307903002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2307903002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:566: context, arg_count, first_arg,
function);
Ah, I misunderstood you in our offline conversation: This actually only makes
sense if you pass the allocation site to the Array constructor, because that
does the elements kind tracking. I'm pretty sure we need this for 2-3
benchmarks, otherwise we don't remove the majority of elements kind transitions
and we might even stay polymophic in many cases.

[mythria, 2016-09-05 12:56:34]

Thanks Benedikt. This cl has implementation for x64. I am also working on ports
to ia32 and Arm.

https://codereview.chromium.org/2307903002/diff/1/src/interpreter/interpreter...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2307903002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:566: context, arg_count, first_arg,
function);
On 2016/09/05 04:11:20, Benedikt Meurer wrote:
> Ah, I misunderstood you in our offline conversation: This actually only makes
> sense if you pass the allocation site to the Array constructor, because that
> does the elements kind tracking. I'm pretty sure we need this for 2-3
> benchmarks, otherwise we don't remove the majority of elements kind
transitions
> and we might even stay polymophic in many cases.

Thanks Benedikt. I now implemented it throw ArrayConstructor Stub. Done.

[mythria, 2016-09-05 12:57:07]

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

[commit-bot: I haz the power, 2016-09-05 12:57: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, 2016-09-05 13:00:48]

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

[commit-bot: I haz the power, 2016-09-05 13:00:49]

Dry run: Try jobs failed on following builders:
  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_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_mips64el_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...)
  v8_linux_mipsel_compile_rel on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...)
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8205)

[Benedikt Meurer, 2016-09-06 03:24:38]

Nice, thanks for addressing this. LGTM once ports are done.

[mythria, 2016-09-06 08:47:46]

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

[commit-bot: I haz the power, 2016-09-06 08:47:54]

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, 2016-09-06 08:56:03]

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

[commit-bot: I haz the power, 2016-09-06 08:56:04]

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

[mythria, 2016-09-06 09:22:54]

Thanks Benedikt. I uploaded ports to ia32, arm and arm64. PTAL.

[rmcilroy, 2016-09-06 10:59:36]

Looks good, a couple of comments.

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/arm64/buil...
File src/builtins/arm64/builtins-arm64.cc (right):

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/arm64/buil...
src/builtins/arm64/builtins-arm64.cc:1177: Register scratch = x5, last_addr =
x6, stack_addr = x7;
please pass these as arguments

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/arm64/buil...
src/builtins/arm64/builtins-arm64.cc:1194: }
nit - could you just do this in InterpreterPushArgsAndConstructImpl before
pushing the args and remove the receiver_available argument, like is done in the
arm builtin.

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/ia32/built...
File src/builtins/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/ia32/built...
src/builtins/ia32/builtins-ia32.cc:769: bool is_receiver_available) {
Could you pass the registers which are used as arguments

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/ia32/built...
src/builtins/ia32/builtins-ia32.cc:834: if (is_receiver_available) {
ditto (if possible)

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

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:793: //           they are to be pushed onto
the stack.
This isn't really applicable here (since we don't use the register names
directly)

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:798: if (push_receiver) {
Ditto (if possible)

https://codereview.chromium.org/2307903002/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2307903002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:542: Node* context_slot =
nit - array_function

https://codereview.chromium.org/2307903002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:586: // Check that it is not the
Array() function.
Update comment - Check if it is the Array() function

https://codereview.chromium.org/2307903002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:621: // TODO(mythria): Inline the
creation of allocation site.
/s/of allocation/of the allocation

https://codereview.chromium.org/2307903002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:636: Callable callable_call =
CodeFactory::InterpreterPushArgsAndCall(
Can you pull out a shared branch for call_function and goto it from here and
after initialization (keep the fast-path monomorphic case inlined though)

[rmcilroy, 2016-09-06 10:59:37]

Looks good, a couple of comments.

[mythria, 2016-09-07 08:59:45]

Thanks Ross. I addressed most of your comments. For ia32 InterprterPushArgs
builtin still takes a flag which tells if receiver is available as a part of
arguments. We do not have any free registers, we cannot push the receiver slot
before we pop the return address. 

PTAL.

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/arm64/buil...
File src/builtins/arm64/builtins-arm64.cc (right):

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/arm64/buil...
src/builtins/arm64/builtins-arm64.cc:1177: Register scratch = x5, last_addr =
x6, stack_addr = x7;
On 2016/09/06 10:59:35, rmcilroy wrote:
> please pass these as arguments

Done.

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/arm64/buil...
src/builtins/arm64/builtins-arm64.cc:1194: }
On 2016/09/06 10:59:35, rmcilroy wrote:
> nit - could you just do this in InterpreterPushArgsAndConstructImpl before
> pushing the args and remove the receiver_available argument, like is done in
the
> arm builtin.

Done.

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/ia32/built...
File src/builtins/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/ia32/built...
src/builtins/ia32/builtins-ia32.cc:769: bool is_receiver_available) {
On 2016/09/06 10:59:35, rmcilroy wrote:
> Could you pass the registers which are used as arguments

Done.

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/ia32/built...
src/builtins/ia32/builtins-ia32.cc:834: if (is_receiver_available) {
On 2016/09/06 10:59:35, rmcilroy wrote:
> ditto (if possible)

This is difficult. We do not have any free registers, and we cannot push
receiver before we pop return address. So I left it as is.

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

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:793: //           they are to be pushed onto
the stack.
On 2016/09/06 10:59:35, rmcilroy wrote:
> This isn't really applicable here (since we don't use the register names
> directly)

Done.

https://codereview.chromium.org/2307903002/diff/60001/src/builtins/x64/builti...
src/builtins/x64/builtins-x64.cc:798: if (push_receiver) {
On 2016/09/06 10:59:35, rmcilroy wrote:
> Ditto (if possible)

Done.

https://codereview.chromium.org/2307903002/diff/60001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.cc (right):

https://codereview.chromium.org/2307903002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:542: Node* context_slot =
On 2016/09/06 10:59:35, rmcilroy wrote:
> nit - array_function

Done.

https://codereview.chromium.org/2307903002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:586: // Check that it is not the
Array() function.
On 2016/09/06 10:59:35, rmcilroy wrote:
> Update comment - Check if it is the Array() function

Done.

https://codereview.chromium.org/2307903002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:621: // TODO(mythria): Inline the
creation of allocation site.
On 2016/09/06 10:59:35, rmcilroy wrote:
> /s/of allocation/of the allocation

Done.

https://codereview.chromium.org/2307903002/diff/60001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:636: Callable callable_call =
CodeFactory::InterpreterPushArgsAndCall(
On 2016/09/06 10:59:35, rmcilroy wrote:
> Can you pull out a shared branch for call_function and goto it from here and
> after initialization (keep the fast-path monomorphic case inlined though)

Done.

[mythria, 2016-09-07 09:02:17]

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

[commit-bot: I haz the power, 2016-09-07 09:02: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, 2016-09-07 09:08:21]

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

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

Dry run: Try jobs failed on following builders:
  v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/2...)
  v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/13837)

[mythria, 2016-09-07 09:21:22]

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

[commit-bot: I haz the power, 2016-09-07 09:21: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, 2016-09-07 09:48:23]

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

[commit-bot: I haz the power, 2016-09-07 09:48:24]

Dry run: This issue passed the CQ dry run.

[rmcilroy, 2016-09-07 10:57:39]

A couple of last comments, but LGTM.

https://codereview.chromium.org/2307903002/diff/100001/src/builtins/arm/built...
File src/builtins/arm/builtins-arm.cc (right):

https://codereview.chromium.org/2307903002/diff/100001/src/builtins/arm/built...
src/builtins/arm/builtins-arm.cc:1167: Register scratch) {
Could you just pass an extra scratch register and use that for limit, rather
than updating num_args and having to copy it to a different register in
InterpreterPushArgsAndConstructImpl? (Same for all architectures)

https://codereview.chromium.org/2307903002/diff/100001/src/builtins/ia32/buil...
File src/builtins/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/2307903002/diff/100001/src/builtins/ia32/buil...
src/builtins/ia32/builtins-ia32.cc:769: // stack, so they should either have an
Smi or a tagged pointer.
Do reg1/reg2 need to be tagged? They are pushed, but aren't they popped again
before we call into the runtime / another function?

https://codereview.chromium.org/2307903002/diff/100001/src/builtins/ia32/buil...
src/builtins/ia32/builtins-ia32.cc:773: Register reg1, Register reg2,
nit - reg1 / reg2 -> scratch1 / scratch2 (also update comment above)

https://codereview.chromium.org/2307903002/diff/100001/src/builtins/ia32/buil...
src/builtins/ia32/builtins-ia32.cc:882: // eax is readonly. ecx will be
modified. edx and edi will be modified but
Capitalize start of sentences (e.g., do "The eax register..." to avoid
capitalizing register name)

[mythria, 2016-09-07 13:05:30]

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

[commit-bot: I haz the power, 2016-09-07 13:05:35]

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

[mythria, 2016-09-07 13:11:16]

Thanks Ross. I addressed all your comments.

Regards,
Mythri

https://codereview.chromium.org/2307903002/diff/100001/src/builtins/arm/built...
File src/builtins/arm/builtins-arm.cc (right):

https://codereview.chromium.org/2307903002/diff/100001/src/builtins/arm/built...
src/builtins/arm/builtins-arm.cc:1167: Register scratch) {
On 2016/09/07 10:57:39, rmcilroy wrote:
> Could you just pass an extra scratch register and use that for limit, rather
> than updating num_args and having to copy it to a different register in
> InterpreterPushArgsAndConstructImpl? (Same for all architectures)

Done.

https://codereview.chromium.org/2307903002/diff/100001/src/builtins/ia32/buil...
File src/builtins/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/2307903002/diff/100001/src/builtins/ia32/buil...
src/builtins/ia32/builtins-ia32.cc:769: // stack, so they should either have an
Smi or a tagged pointer.
On 2016/09/07 10:57:39, rmcilroy wrote:
> Do reg1/reg2 need to be tagged? They are pushed, but aren't they popped again
> before we call into the runtime / another function?

As mentioned offline, I was thinking about interrupts. Since it does not matter
for them, I removed this comment. Done.

https://codereview.chromium.org/2307903002/diff/100001/src/builtins/ia32/buil...
src/builtins/ia32/builtins-ia32.cc:773: Register reg1, Register reg2,
On 2016/09/07 10:57:39, rmcilroy wrote:
> nit - reg1 / reg2 -> scratch1 / scratch2 (also update comment above)

Done.

https://codereview.chromium.org/2307903002/diff/100001/src/builtins/ia32/buil...
src/builtins/ia32/builtins-ia32.cc:882: // eax is readonly. ecx will be
modified. edx and edi will be modified but
On 2016/09/07 10:57:39, rmcilroy wrote:
> Capitalize start of sentences (e.g., do "The eax register..." to avoid
> capitalizing register name)

Done.

[commit-bot: I haz the power, 2016-09-07 13:14:04]

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

[commit-bot: I haz the power, 2016-09-07 13:14:05]

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

[mythria, 2016-09-07 13:21:47]

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

[commit-bot: I haz the power, 2016-09-07 13:21:54]

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, 2016-09-07 13:56:58]

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

[commit-bot: I haz the power, 2016-09-07 13:56:59]

Dry run: Try jobs failed on following builders:
  v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/8400)
  v8_mac_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng_triggered/bui...)

[mythria, 2016-09-08 12:59:02]

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

[commit-bot: I haz the power, 2016-09-08 12:59: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, 2016-09-08 13:19:07]

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

[commit-bot: I haz the power, 2016-09-08 13:19:08]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-09-08 14:06:40]

The CQ bit was checked by mythria@chromium.org

[mythria, 2016-09-08 14:06:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, rmcilroy@chromium.org
Link to the patchset: https://codereview.chromium.org/2307903002/#ps120002
(title: "Fixed a bug in mips implementation.")

[commit-bot: I haz the power, 2016-09-08 14:06:51]

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, 2016-09-08 14:49:31]

Committed patchset #9 (id:120002)

[commit-bot: I haz the power, 2016-09-08 14:50:17]

Description was changed from

==========
[Interpreter] Collect allocation site feedback in call bytecode handler.

Adds support to collect allocation site feedback for Array function calls
to the call bytecode handler.

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

to

==========
[Interpreter] Collect allocation site feedback in call bytecode handler.

Adds support to collect allocation site feedback for Array function calls
to the call bytecode handler.

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

Committed: https://crrev.com/9a31162d9d3137d09063d6040865655b2e386384
Cr-Commit-Position: refs/heads/master@{#39283}
==========

[commit-bot: I haz the power, 2016-09-08 14:50:19]

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