[Interpreter] Collect type feedback for 'new' in the bytecode handler

[Interpreter] Collect type feedback for 'new' in the bytecode handler

Collect type feedback in the bytecode handler for 'new' bytecode. The
earlier cl (https://codereview.chromium.org/2153433002/) was reverted
because that implementation did not collect allocation site feedback.
This regressed delta blue by an order of magnitude. This implementation
includes collection of allocation site feedback.

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

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

[mythria, 2016-07-29 08:11:08]

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

[commit-bot: I haz the power, 2016-07-29 08:11:19]

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-07-29 08:33:11]

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

[commit-bot: I haz the power, 2016-07-29 08:33:12]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-07-29 08:33:21]

mythria@chromium.org changed reviewers:
+ balazs.kilvady@imgtec.com, bmeurer@chromium.org, rmcilroy@chromium.org

[mythria, 2016-07-29 08:33:22]

Benedikt, Ross
This cl adds allocation site feedback along with the type feedback for the new
bytecode handler. I am sorry for the delay. I was trying to debug a windows ia32
failure on bots. I could not reproduce it locally. So it took a lot of time to
debug. Thanks to Michi, who pointed the problem.

When debugging this problem, I also realized we need to do a stack overflow
check in both call and construct. I will do it in a different cl. I added a TODO
for it. 
PTAL.

Michi,
I added the code to touch stack pages in the builtin directly. I will move it to
macro assembler in a different patch. If you have time, PTAL at these changes in
builtins-ia32.cc

Thanks,
Mythri

[rmcilroy, 2016-08-03 13:14:19]

lgtm, thanks!

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

https://codereview.chromium.org/2190293003/diff/1/src/builtins/ia32/builtins-...
src/builtins/ia32/builtins-ia32.cc:744: // TODO(mythria): Add a stack check
before pushing the arguments.
We should do this on all architectures (I'm sure you planned to, just pointing
it out since you added the TODO here).

https://codereview.chromium.org/2190293003/diff/1/src/builtins/ia32/builtins-...
src/builtins/ia32/builtins-ia32.cc:802: // (mimimum page size is 4KB) without
accessing atleast one byte on the
atleast/at least

https://codereview.chromium.org/2190293003/diff/1/src/builtins/ia32/builtins-...
src/builtins/ia32/builtins-ia32.cc:819: // Step 1 - Update the stack pointer.
nit - newline above

https://codereview.chromium.org/2190293003/diff/1/src/builtins/ia32/builtins-...
src/builtins/ia32/builtins-ia32.cc:824: // destination overlap. We atleast need
one extra slot for receiver,
/s/atleast/at least

https://codereview.chromium.org/2190293003/diff/1/src/builtins/ia32/builtins-...
src/builtins/ia32/builtins-ia32.cc:849: __ j(greater, &loop_header,
Label::kNear);
Can we do this with Generate_InterpreterPushArgs instead (perhaps recalculating
array_limit beforehand if necessary)?

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

https://codereview.chromium.org/2190293003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:613: // Slot id of 0 is used to
indicate no typefeedback is available.
/s/typefeedback/type feedback/

https://codereview.chromium.org/2190293003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:629: // Cache the called function in a
feedback vector slot.  Cache states
Add scope for binded label.

https://codereview.chromium.org/2190293003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:629: // Cache the called function in a
feedback vector slot.  Cache states
Remove extra space after fullstop

https://codereview.chromium.org/2190293003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:635: // fast path.
Could you check this again now that you've moved the increment_count down it
might be preferable that extra_checks is deferred if it doesn't have the
register spilling issue.

https://codereview.chromium.org/2190293003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:704: // Check that it is not the
Array() function.
nit - Check if it is the Array() function.

[Benedikt Meurer, 2016-08-03 18:18:37]

Nice. LGTM.

[mythria, 2016-08-04 06:46:16]

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

[commit-bot: I haz the power, 2016-08-04 06:46:20]

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-08-04 06:47:55]

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

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

Dry run: Try jobs failed on following builders:
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10306)
  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_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/10292)
  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_mac_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/6298)

[mythria, 2016-08-04 07:06:11]

Thanks for your reviews. Fixed all the  nits. 
It is not possible to reuse InterpreterPushArgs for construct in ia-32. I fixed
all others.

Thanks,
Mythri

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

https://codereview.chromium.org/2190293003/diff/1/src/builtins/ia32/builtins-...
src/builtins/ia32/builtins-ia32.cc:744: // TODO(mythria): Add a stack check
before pushing the arguments.
On 2016/08/03 13:14:19, rmcilroy wrote:
> We should do this on all architectures (I'm sure you planned to, just pointing
> it out since you added the TODO here).

Yes, I was lazy to add TODO on all architectures. Added them for Arm and x64.
Acknowledged.

https://codereview.chromium.org/2190293003/diff/1/src/builtins/ia32/builtins-...
src/builtins/ia32/builtins-ia32.cc:802: // (mimimum page size is 4KB) without
accessing atleast one byte on the
On 2016/08/03 13:14:19, rmcilroy wrote:
> atleast/at least

Done.

https://codereview.chromium.org/2190293003/diff/1/src/builtins/ia32/builtins-...
src/builtins/ia32/builtins-ia32.cc:824: // destination overlap. We atleast need
one extra slot for receiver,
On 2016/08/03 13:14:19, rmcilroy wrote:
> /s/atleast/at least

Done.

https://codereview.chromium.org/2190293003/diff/1/src/builtins/ia32/builtins-...
src/builtins/ia32/builtins-ia32.cc:849: __ j(greater, &loop_header,
Label::kNear);
On 2016/08/03 13:14:19, rmcilroy wrote:
> Can we do this with Generate_InterpreterPushArgs instead (perhaps
recalculating
> array_limit beforehand if necessary)?

InterpreterPushArgs pushes the arguments onto stack, we copy to the correct
location. Here we need source address, destination address and last address, so
3 registers instead of 2. (esp was the destination earlier, so we need only 2).
I can shift to the scheme where we compute the last address so we can use a
simpler addressing mode, but we need another register. One more push and pop.
Even in that case, we can't reuse InterpreterPushArgs.

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

https://codereview.chromium.org/2190293003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:613: // Slot id of 0 is used to
indicate no typefeedback is available.
On 2016/08/03 13:14:19, rmcilroy wrote:
> /s/typefeedback/type feedback/

Done.

https://codereview.chromium.org/2190293003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:629: // Cache the called function in a
feedback vector slot.  Cache states
On 2016/08/03 13:14:19, rmcilroy wrote:
> Remove extra space after fullstop

Done.

https://codereview.chromium.org/2190293003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:629: // Cache the called function in a
feedback vector slot.  Cache states
On 2016/08/03 13:14:19, rmcilroy wrote:
> Add scope for binded label.

Done.

https://codereview.chromium.org/2190293003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:635: // fast path.
On 2016/08/03 13:14:19, rmcilroy wrote:
> Could you check this again now that you've moved the increment_count down it
> might be preferable that extra_checks is deferred if it doesn't have the
> register spilling issue.

It still has. so did not mark it deferred.

https://codereview.chromium.org/2190293003/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:704: // Check that it is not the
Array() function.
On 2016/08/03 13:14:19, rmcilroy wrote:
> nit - Check if it is the Array() function.

Done.

[mythria, 2016-08-04 07:13:40]

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

[commit-bot: I haz the power, 2016-08-04 07:13:43]

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-08-04 07:26:29]

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

[commit-bot: I haz the power, 2016-08-04 07:26:30]

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

[mythria, 2016-08-05 05:55:32]

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

[commit-bot: I haz the power, 2016-08-05 05:55:38]

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-08-05 05:57:09]

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

[commit-bot: I haz the power, 2016-08-05 05:57:10]

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_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  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_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/10378)
  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/10364)
  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_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/10322)
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/21063)

[mythria, 2016-08-05 06:35:53]

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

[commit-bot: I haz the power, 2016-08-05 06:35:57]

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-08-05 06:49:26]

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

[commit-bot: I haz the power, 2016-08-05 06:49:27]

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

[mythria, 2016-08-05 07:23:24]

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

[commit-bot: I haz the power, 2016-08-05 07:23:26]

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-08-05 07:48:59]

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

[commit-bot: I haz the power, 2016-08-05 07:48:59]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-08-05 08:55:04]

I updated cctest and mjsunit sttus files. Two additional tests pass because we
have allocation site feedback. There are few more which still fail because we
disable mementos when creating array literals. I marked them so it is easy to
check when we enable mementos. 

Thanks,
Mythri

[mythria, 2016-08-05 09:56:05]

The CQ bit was checked by mythria@chromium.org

[mythria, 2016-08-05 09:56:05]

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/2190293003/#ps100001
(title: "updated cctest.status and mjsunit.status")

[commit-bot: I haz the power, 2016-08-05 09:56:10]

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-08-05 09:57:54]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-05 09:58:51]

Description was changed from

==========
[Interpreter] Collect type feedback for 'new' in the bytecode handler

Collect type feedback in the bytecode handler for 'new' bytecode. The
earlier cl (https://codereview.chromium.org/2153433002/) was reverted
because that implementation did not collect allocation site feedback.
This regressed delta blue by an order of magnitude. This implementation
includes collection of allocation site feedback.

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

to

==========
[Interpreter] Collect type feedback for 'new' in the bytecode handler

Collect type feedback in the bytecode handler for 'new' bytecode. The
earlier cl (https://codereview.chromium.org/2153433002/) was reverted
because that implementation did not collect allocation site feedback.
This regressed delta blue by an order of magnitude. This implementation
includes collection of allocation site feedback.

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

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

[commit-bot: I haz the power, 2016-08-05 09:58:53]

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

[Michael Achenbach, 2016-08-05 10:32:56]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/2212343002/ by machenbach@chromium.org.

The reason for reverting is: [Sheriff] Fails on nosnap debug:
https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20nosnap%20-%2....