[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
current implementation does not collect allocation site feedback.

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

Committed: https://crrev.com/1eadc76419b323fb2e55ae9953142f801704aa59
Cr-Commit-Position: refs/heads/master@{#37862}

[mythria, 2016-07-14 14:08:06]

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

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

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-14 14:35:59]

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

[commit-bot: I haz the power, 2016-07-14 14:36:00]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-07-14 15:00:55]

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

[mythria, 2016-07-14 15:00:56]

I added type feedback support for new bytecode. I did not add support to collect
allocation site feedback. PTAL.

Thanks,
Mythri

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

https://codereview.chromium.org/2153433002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:652: BranchIf(is_smi, &initialize,
&mark_megamorphic);
This is a bit different from Call type feedback. We don't check if a weak cell
is cleared in the case of Call.

https://codereview.chromium.org/2153433002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:712: BranchIf(is_js_function,
&js_function, &call_constrcut);
If constructor is not a JSFunction, we do not update the type feedback. Is this
correct? For Call bytecode we move to megamorphic, so I just wanted to make sure
this is intended.

[rmcilroy, 2016-07-15 10:44:06]

Looks good overall. A couple of comments. Also, we should update
bytecode-graph-builder to pass the real feedback vector slot to the
CallConstruct node as well.

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

https://codereview.chromium.org/2153433002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:604: void
InterpreterAssembler::UpdateTypeFeedback(Node* slot_id,
Could we just do this inline? Or at least make the function name specific to
CallConstruct

https://codereview.chromium.org/2153433002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:712: BranchIf(is_js_function,
&js_function, &call_constrcut);
call_constrcut - fix typo

[mythria, 2016-07-15 13:21:57]

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

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

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

[mythria, 2016-07-15 13:53:41]

Thanks Ross. I forgot about passing the correct feedback to the bytecode graph
builder. Fixed it. I fixed other as well.
PTAL.

Thanks,
Mythri

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

https://codereview.chromium.org/2153433002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:604: void
InterpreterAssembler::UpdateTypeFeedback(Node* slot_id,
On 2016/07/15 10:44:06, rmcilroy wrote:
> Could we just do this inline? Or at least make the function name specific to
> CallConstruct

I inlined it into the callConstruct. Done.

https://codereview.chromium.org/2153433002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:712: BranchIf(is_js_function,
&js_function, &call_constrcut);
On 2016/07/15 10:44:06, rmcilroy wrote:
> call_constrcut - fix typo

Thanks. Done.

[commit-bot: I haz the power, 2016-07-15 14:51:31]

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

[commit-bot: I haz the power, 2016-07-15 14:51:34]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-07-17 19:37:14]

Thanks Mythri. LGTM with one comment.

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

https://codereview.chromium.org/2153433002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:692: CreateWeakCellStub
weak_cell_stub(isolate());
Can you place a TODO here, that we want to inline the weak cell
creation/registration here at some point? (same for the Call).

[mythria, 2016-07-18 08:47:17]

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

[commit-bot: I haz the power, 2016-07-18 08:47:29]

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-18 09:16:27]

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

[commit-bot: I haz the power, 2016-07-18 09:16:28]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-07-18 09:22:40]

Thanks for the review Benedikt. I added the TODO to both call and construct
handlers.

Regards,
Mythri

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

https://codereview.chromium.org/2153433002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:692: CreateWeakCellStub
weak_cell_stub(isolate());
On 2016/07/17 19:37:13, Benedikt Meurer wrote:
> Can you place a TODO here, that we want to inline the weak cell
> creation/registration here at some point? (same for the Call).

Thanks Benedikt. I can look at it after I have done the BinaryOp feedback. Done.

[rmcilroy, 2016-07-18 10:13:01]

LGTM with a couple of comments, thanks.

Benedikt: Do you have any answers for the question's Mythri origionally had on
the CL:

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

https://codereview.chromium.org/2153433002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:652: BranchIf(is_smi, &initialize,
&mark_megamorphic);
This is a bit different from Call type feedback. We don't check if a weak cell
is cleared in the case of Call.

https://codereview.chromium.org/2153433002/diff/1/src/interpreter/interpreter...
src/interpreter/interpreter-assembler.cc:712: BranchIf(is_js_function,
&js_function, &call_constrcut);
If constructor is not a JSFunction, we do not update the type feedback. Is this
correct? For Call bytecode we move to megamorphic, so I just wanted to make sure
this is intended.

https://codereview.chromium.org/2153433002/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2153433002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1071:
CreateVectorSlotPair(bytecode_iterator().GetIndexOperand(3));
nit - could you move this to the top where other operands are being read.

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

https://codereview.chromium.org/2153433002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:631: Label increment_count(this),
extra_checks(this),
Should extra_checks be kDeferred ?

https://codereview.chromium.org/2153433002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:655: check_weak_cell_cleared(this);
Should any of these be kDeferred?

[mythria, 2016-07-19 09:50:59]

Thanks Ross, as discussed offline, I added a TODO for marking the extra_checks
block as deferred block. If I mark it as deferred the generated code (on x64)
contains lot of register swaps which could be avoided. I also filed a bug for
the same.

Thanks,
Mythri

https://codereview.chromium.org/2153433002/diff/40001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.cc (right):

https://codereview.chromium.org/2153433002/diff/40001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:1071:
CreateVectorSlotPair(bytecode_iterator().GetIndexOperand(3));
On 2016/07/18 10:13:01, rmcilroy wrote:
> nit - could you move this to the top where other operands are being read.

Done.

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

https://codereview.chromium.org/2153433002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:631: Label increment_count(this),
extra_checks(this),
On 2016/07/18 10:13:01, rmcilroy wrote:
> Should extra_checks be kDeferred ?

As discussed offline, making any of the blocks kDeferred so that we have the
fast path with less branch penalty would generate several register spills. Added
a comment and filed a bug for the same.

https://codereview.chromium.org/2153433002/diff/40001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:655: check_weak_cell_cleared(this);
On 2016/07/18 10:13:01, rmcilroy wrote:
> Should any of these be kDeferred? 

I think Deferred label for extra_checks would be sufficient. Done.

[mythria, 2016-07-19 09:51:06]

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

[commit-bot: I haz the power, 2016-07-19 09:51: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-07-19 09:52:42]

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

[commit-bot: I haz the power, 2016-07-19 09:52:44]

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_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_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/9263)
  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_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/19725)

[mythria, 2016-07-19 10:21:55]

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

[commit-bot: I haz the power, 2016-07-19 10:22:03]

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-19 10:45:45]

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

[commit-bot: I haz the power, 2016-07-19 10:45:46]

Dry run: This issue passed the CQ dry run.

[mythria, 2016-07-19 11:07:30]

The CQ bit was checked by mythria@chromium.org

[mythria, 2016-07-19 11:07:30]

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/2153433002/#ps100001
(title: "rebase.")

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

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-19 11:10:00]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-07-19 11:10:42]

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
current implementation does not collect 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
current implementation does not collect allocation site feedback.

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

Committed: https://crrev.com/1eadc76419b323fb2e55ae9953142f801704aa59
Cr-Commit-Position: refs/heads/master@{#37862}
==========

[commit-bot: I haz the power, 2016-07-19 11:10:43]

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

[mythria, 2016-07-19 14:15:49]

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

The reason for reverting is: This cl causes a large regression in octane
(https://chromeperf.appspot.com/group_report?bug_id=629503). I have to
investigate the reason before I can reland this..