[Interpreter] Collect type feedback for calls in the bytecode handler

[Interpreter] Collect type feedback for calls in the bytecode handler

Collect type feedback in the call bytecode handler. The current
implementation only collects feedback for JS function objects. The other
objects and Array functions do not collect any feedback. They will be
marked Megamorphic.

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

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

[mythria, 2016-07-06 13:33:12]

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

[commit-bot: I haz the power, 2016-07-06 13:33:23]

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-06 13:37:10]

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

[commit-bot: I haz the power, 2016-07-06 13:37:11]

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

[mythria, 2016-07-07 08:18:35]

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

[mythria, 2016-07-07 08:18:36]

I added support to collect type feedback to the call bytecode handler. As
discussed, I do not handle arrays here. I also handle everything in the handler
and not call into the runtime for handling a miss. For now I have only modified
the InterpreterPushArgs builtin for x64. If this looks ok I will port it to
other architectures as well. PTAL. 

Thanks,
Mythri

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

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:567: SKIP_WRITE_BARRIER);
MegamorphicSentinel is created here: Heap::CreateInitialObjects(). So I am
assuming it does not move during a GC and is a strong root so we do not need a
write barrier. I think CallICStub also does not use a write barrier for storing
megamorphic sentinel. I am not so confident if it is ok to skip the barrier. Is
it safer to have it?

[rmcilroy, 2016-07-08 09:58:00]

Looks good overall. A couple of comments.

https://codereview.chromium.org/2122183002/diff/20001/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/2122183002/diff/20001/src/builtins.h#newcode456
src/builtins.h:456: bool is_js_function = false);
nit - could you make this bool an enum instead.

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

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:441: Node*
InterpreterAssembler::CallJSWithFeedback(Node* function, Node* context,
As discussed offline, if slot '0' is not a valid slot then we could avoid the
extra bytecodes and just check whether the slot is non-zero to decide whether to
update the TFV or not.

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:443: Node* slot_id_smi,
You could pass slot_id as a non-smi, to avoid untagging on line 464, and just
tag before calling the stub on line 545

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:529: GotoIf(is_array_function,
&mark_megamorphic);
Benedikt, just want to check that it is correct to go megamorphic if this is the
Array() function?

https://codereview.chromium.org/2122183002/diff/20001/test/cctest/cctest.status
File test/cctest/cctest.status (right):

https://codereview.chromium.org/2122183002/diff/20001/test/cctest/cctest.stat...
test/cctest/cctest.status:151: # TODO(mythria,4780): Related to type feedback
support for calls.
Now that we have type feedback vector support, any idea why the remainder of
these are still failing?

[Yang, 2016-07-08 10:39:15]

On 2016/07/08 09:58:00, rmcilroy wrote:
> Looks good overall. A couple of comments.
> 
> https://codereview.chromium.org/2122183002/diff/20001/src/builtins.h
> File src/builtins.h (right):
> 
>
https://codereview.chromium.org/2122183002/diff/20001/src/builtins.h#newcode456
> src/builtins.h:456: bool is_js_function = false);
> nit - could you make this bool an enum instead.
> 
>
https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
> File src/interpreter/interpreter-assembler.cc (right):
> 
>
https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
> src/interpreter/interpreter-assembler.cc:441: Node*
> InterpreterAssembler::CallJSWithFeedback(Node* function, Node* context,
> As discussed offline, if slot '0' is not a valid slot then we could avoid the
> extra bytecodes and just check whether the slot is non-zero to decide whether
to
> update the TFV or not.
> 
>
https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
> src/interpreter/interpreter-assembler.cc:443: Node* slot_id_smi,
> You could pass slot_id as a non-smi, to avoid untagging on line 464, and just
> tag before calling the stub on line 545
> 
>
https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
> src/interpreter/interpreter-assembler.cc:529: GotoIf(is_array_function,
> &mark_megamorphic);
> Benedikt, just want to check that it is correct to go megamorphic if this is
the
> Array() function?
> 
>
https://codereview.chromium.org/2122183002/diff/20001/test/cctest/cctest.status
> File test/cctest/cctest.status (right):
> 
>
https://codereview.chromium.org/2122183002/diff/20001/test/cctest/cctest.stat...
> test/cctest/cctest.status:151: # TODO(mythria,4780): Related to type feedback
> support for calls.
> Now that we have type feedback vector support, any idea why the remainder of
> these are still failing?

src/debug lgtm.

[Benedikt Meurer, 2016-07-09 18:19:22]

Hey Mythri,

Looks promising, thanks for the hard work, and sorry for the delayed review. I
think this is almost landable and should already help somewhat for the
benchmarks where we currently lack Call feedback.

-- Benedikt

https://codereview.chromium.org/2122183002/diff/20001/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/2122183002/diff/20001/src/builtins.h#newcode456
src/builtins.h:456: bool is_js_function = false);
Yes, please.

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

https://codereview.chromium.org/2122183002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:981: // TODO(rmcilroy): Set receiver_hint
correctly based on whether the receiver
The JSTypedLowering will figure it out based on types. So unless you can provide
a better hint, there's no point in doing this.

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2529: DCHECK(call_type !=
Call::POSSIBLY_EVAL_CALL);
Nit: Use DCHECK_NE.

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

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:529: GotoIf(is_array_function,
&mark_megamorphic);
Yes, it's not ideal, but it's correct (and the simplest initial step). This way,
the Array constructor will always be called in the regular way (without passing
an AllocationSite).

The next step will be to extend this code to also be able to call directly into
the Array constructor and pass an AllocationSite (shouldn't be too hard now that
all the basics are there), since we unfortunately still depend on the
AllocationSite feedback for the elements kind transitions.

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:567: SKIP_WRITE_BARRIER);
Yes, it's part of the immortal, immovable roots, and we depend on that being the
case. So it's always OK to skip the write barrier. If you want to play it
super-safe, then you can DCHECK that the sentinel is an immortal immovable root
(the Heap has a function for that).

[rmcilroy, 2016-07-11 09:28:27]

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

https://codereview.chromium.org/2122183002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:981: // TODO(rmcilroy): Set receiver_hint
correctly based on whether the receiver
On 2016/07/09 18:19:22, Benedikt Meurer wrote:
> The JSTypedLowering will figure it out based on types. So unless you can
provide
> a better hint, there's no point in doing this.

This was based on the ASTGraphBuilder where it passes ConvertReceiverMode
depending on whether it is building a global, property or eval/other call. We
could possibly plumb this knowledge through in the bytecode if it is useful, but
if we would be able to get the same knowledge from the types that would be
great.

[mythria, 2016-07-11 13:33:17]

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

[commit-bot: I haz the power, 2016-07-11 13:33: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-11 13:38:49]

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

[commit-bot: I haz the power, 2016-07-11 13:38:50]

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-07-11 14:50:07]

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

[commit-bot: I haz the power, 2016-07-11 14:50:18]

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-11 14:56:08]

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

[commit-bot: I haz the power, 2016-07-11 14:56:09]

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-07-11 15:25:25]

Thanks Benedikt and Ross. I addressed your comments and also ported it to
ia32/arm/arm64.

PTAL.

Thanks,
Mythri

https://codereview.chromium.org/2122183002/diff/20001/src/builtins.h
File src/builtins.h (right):

https://codereview.chromium.org/2122183002/diff/20001/src/builtins.h#newcode456
src/builtins.h:456: bool is_js_function = false);
On 2016/07/09 18:19:22, Benedikt Meurer wrote:
> Yes, please.

Thanks, Done.

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

https://codereview.chromium.org/2122183002/diff/20001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.cc:981: // TODO(rmcilroy): Set receiver_hint
correctly based on whether the receiver
On 2016/07/11 09:28:27, rmcilroy wrote:
> On 2016/07/09 18:19:22, Benedikt Meurer wrote:
> > The JSTypedLowering will figure it out based on types. So unless you can
> provide
> > a better hint, there's no point in doing this.
> 
> This was based on the ASTGraphBuilder where it passes ConvertReceiverMode
> depending on whether it is building a global, property or eval/other call. We
> could possibly plumb this knowledge through in the bytecode if it is useful,
but
> if we would be able to get the same knowledge from the types that would be
> great.

I removed the TODO. I will reintroduce it back if we have to provide additional
hints similar to ASTGraphBuilder.

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2529: DCHECK(call_type !=
Call::POSSIBLY_EVAL_CALL);
On 2016/07/09 18:19:22, Benedikt Meurer wrote:
> Nit: Use DCHECK_NE.

This code changed a little and I moved DCHECK to the if part. Done.

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

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:441: Node*
InterpreterAssembler::CallJSWithFeedback(Node* function, Node* context,
On 2016/07/08 09:57:59, rmcilroy wrote:
> As discussed offline, if slot '0' is not a valid slot then we could avoid the
> extra bytecodes and just check whether the slot is non-zero to decide whether
to
> update the TFV or not.

Done.

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:443: Node* slot_id_smi,
On 2016/07/08 09:57:59, rmcilroy wrote:
> You could pass slot_id as a non-smi, to avoid untagging on line 464, and just
> tag before calling the stub on line 545

Done.

https://codereview.chromium.org/2122183002/diff/20001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:567: SKIP_WRITE_BARRIER);
On 2016/07/09 18:19:22, Benedikt Meurer wrote:
> Yes, it's part of the immortal, immovable roots, and we depend on that being
the
> case. So it's always OK to skip the write barrier. If you want to play it
> super-safe, then you can DCHECK that the sentinel is an immortal immovable
root
> (the Heap has a function for that).

Thanks Benedikt. I added a DCHECK more to make the assumption explicit.

https://codereview.chromium.org/2122183002/diff/20001/test/cctest/cctest.status
File test/cctest/cctest.status (right):

https://codereview.chromium.org/2122183002/diff/20001/test/cctest/cctest.stat...
test/cctest/cctest.status:151: # TODO(mythria,4780): Related to type feedback
support for calls.
On 2016/07/08 09:58:00, rmcilroy wrote:
> Now that we have type feedback vector support, any idea why the remainder of
> these are still failing?

They still fail because we do not call through CallIC or use CallNexus, they
check if the CallNexus has the correct number and correct type of entries. May
be we can modify the tests for ignition.

[rmcilroy, 2016-07-11 15:58:55]

https://codereview.chromium.org/2122183002/diff/20001/test/cctest/cctest.status
File test/cctest/cctest.status (right):

https://codereview.chromium.org/2122183002/diff/20001/test/cctest/cctest.stat...
test/cctest/cctest.status:151: # TODO(mythria,4780): Related to type feedback
support for calls.
On 2016/07/11 15:25:24, mythria wrote:
> On 2016/07/08 09:58:00, rmcilroy wrote:
> > Now that we have type feedback vector support, any idea why the remainder of
> > these are still failing?
> 
> They still fail because we do not call through CallIC or use CallNexus, they
> check if the CallNexus has the correct number and correct type of entries. May
> be we can modify the tests for ignition. 

Yes, I think we should modify the tests to test the equivalent things for
Ignition if it is possible.

[Benedikt Meurer, 2016-07-12 03:25:35]

+1 on tests.

Otherwise LGTM.

[mythria, 2016-07-12 09:01:33]

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

[mythria, 2016-07-12 09:01:34]

Hi Michael,
As discussed offline, I use slot id of 0 to indicate we don't need any type
feedback. This is just an understanding between bytecode generator and the
bytecode handler. The changes for this are in bytecode-generator.cc and
interpreter-assembler.cc. PTAL.

Benedikt, Ross,
I am sorry I did not look at the tests carefully. One of them fails because it
checks for type feedback for construct and the other because it checks for type
feedback for Array function. I updated the status file with this information.
PTAL. 

Thanks,
Mythri

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2532: feedback_slot_index = 0;
@mvstanton, this is where we pass 0 for invalid slot id. As discussed offline I
added a static assert. PTAL.

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

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.cc:468: GotoIf(is_feedback_unavailable,
&call);
@mvstanton: I use slot id 0 to indicate we don't need type feedback. As
discussed offline, I added a static assert. PTAL.

https://codereview.chromium.org/2122183002/diff/80001/test/cctest/cctest.status
File test/cctest/cctest.status (right):

https://codereview.chromium.org/2122183002/diff/80001/test/cctest/cctest.stat...
test/cctest/cctest.status:152: 'test-feedback-vector/VectorCallICStates': [PASS,
NO_IGNITION],
This fails because it checks for type feedback for Array function. Since we plan
to do it in future, I just updated the comment to keep track of it.

https://codereview.chromium.org/2122183002/diff/80001/test/cctest/cctest.stat...
test/cctest/cctest.status:155: 'test-feedback-vector/VectorCallCounts': [PASS,
NO_IGNITION],
This fails because it also checks for type feedback from new along with call. I
will address the type feedback for construct next. It should work then.

[rmcilroy, 2016-07-12 09:36:21]

LGTM with some comments, thanks!

https://codereview.chromium.org/2122183002/diff/80001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/2122183002/diff/80001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:1171: } else {
DCHECK_EQ(function_type, CallableType:kAny (for all ports).

https://codereview.chromium.org/2122183002/diff/80001/src/compiler/bytecode-g...
File src/compiler/bytecode-graph-builder.h (right):

https://codereview.chromium.org/2122183002/diff/80001/src/compiler/bytecode-g...
src/compiler/bytecode-graph-builder.h:125: void
BuildCallWithFeedback(TailCallMode tail_call_mode);
Remove

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

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:307: static Bytecode
BytecodeForCallWithFeedback(TailCallMode tail_call_mode);
Remove

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2532: feedback_slot_index = 0;
I think feedback_slot_index will already be zero for an invalid slot. Could we
just do:

int feedback_slot_index = feedback_index(expr->CallFeedbackICSlot());

// Comment...
STATIC_ASSERT(TypeFeedbackVector::kReservedIndexCount > 0);
DCHECK(expr->CallFeedbackICSlot().Valid() || (
       (feedback_slot_index == 0 && call_type == Call::POSSIBLY_EVAL_CALL));

?

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecodes.h (right):

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecodes.h:599: static bool IsTailCall(Bytecode bytecode);
Remove

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/interpr...
File src/interpreter/interpreter-assembler.h (left):

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/interpr...
src/interpreter/interpreter-assembler.h:98: 
nit - add back newline

https://codereview.chromium.org/2122183002/diff/80001/test/cctest/cctest.status
File test/cctest/cctest.status (right):

https://codereview.chromium.org/2122183002/diff/80001/test/cctest/cctest.stat...
test/cctest/cctest.status:152: 'test-feedback-vector/VectorCallICStates': [PASS,
NO_IGNITION],
On 2016/07/12 09:01:34, mythria wrote:
> This fails because it checks for type feedback for Array function. Since we
plan
> to do it in future, I just updated the comment to keep track of it.

As discussed offline, maybe we could split this into two tests (one for
construct, one without.

https://codereview.chromium.org/2122183002/diff/80001/test/cctest/cctest.stat...
test/cctest/cctest.status:155: 'test-feedback-vector/VectorCallCounts': [PASS,
NO_IGNITION],
On 2016/07/12 09:01:34, mythria wrote:
> This fails because it also checks for type feedback from new along with call.
I
> will address the type feedback for construct next. It should work then.

As discussed offline, maybe we could split this into two tests (one for
construct, one without.

[mythria, 2016-07-12 10:33:41]

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

[commit-bot: I haz the power, 2016-07-12 10:33:42]

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-12 10:37:34]

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

[commit-bot: I haz the power, 2016-07-12 10:37:35]

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/8921)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/8862)

[mythria, 2016-07-12 10:48:22]

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

[commit-bot: I haz the power, 2016-07-12 10:48:32]

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-12 10:51:02]

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

[commit-bot: I haz the power, 2016-07-12 10:51:03]

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/8923)
  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/8893)
  v8_linux_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_rel_ng/builds/8864)

[mythria, 2016-07-12 11:57:04]

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

[commit-bot: I haz the power, 2016-07-12 11:57: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-12 12:13:16]

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

[commit-bot: I haz the power, 2016-07-12 12:13:17]

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

[mvstanton, 2016-07-12 13:25:22]

I just looked at the feedback slot, LGTM on that.

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2532: feedback_slot_index = 0;
On 2016/07/12 09:01:34, mythria wrote:
> @mvstanton, this is where we pass 0 for invalid slot id. As discussed offline
I
> added a static assert. PTAL.

Sounds good.

[mythria, 2016-07-12 13:30:17]

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

[commit-bot: I haz the power, 2016-07-12 13:30: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-07-12 13:47:11]

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

[commit-bot: I haz the power, 2016-07-12 13:47:12]

Dry run: Try jobs failed on following builders:
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/8907)
  v8_linux_dbg_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng_triggered/b...)
  v8_win_nosnap_shared_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)
  v8_win_nosnap_shared_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_win_nosnap_shared_rel_ng...)

[mythria, 2016-07-12 14:00:41]

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

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

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-12 14:16:53]

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

[commit-bot: I haz the power, 2016-07-12 14:16:53]

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

[mythria, 2016-07-12 15:20:35]

Thank you all. I addressed comments from Ross and update the tests by
refactoring the tests. 

test-feedback-vector/VectorCallCounts test is split into two tests one to check
for call counts for call and the other to  check counts for construct. Also
test-feedback-vector/VectorCallICStates now tests only for call type feedback.
Type feedback for Array function is moved to a new test.

PTAL.

Thanks,
Mythri

https://codereview.chromium.org/2122183002/diff/80001/src/arm/builtins-arm.cc
File src/arm/builtins-arm.cc (right):

https://codereview.chromium.org/2122183002/diff/80001/src/arm/builtins-arm.cc...
src/arm/builtins-arm.cc:1171: } else {
On 2016/07/12 09:36:20, rmcilroy wrote:
> DCHECK_EQ(function_type, CallableType:kAny (for all ports).

Done.

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

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-array-builder.h:307: static Bytecode
BytecodeForCallWithFeedback(TailCallMode tail_call_mode);
On 2016/07/12 09:36:20, rmcilroy wrote:
> Remove

Done.

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/bytecod...
File src/interpreter/bytecode-generator.cc (right):

https://codereview.chromium.org/2122183002/diff/80001/src/interpreter/bytecod...
src/interpreter/bytecode-generator.cc:2532: feedback_slot_index = 0;
On 2016/07/12 09:36:20, rmcilroy wrote:
> I think feedback_slot_index will already be zero for an invalid slot. Could we
> just do:
> 
> int feedback_slot_index = feedback_index(expr->CallFeedbackICSlot());
> 
> // Comment...
> STATIC_ASSERT(TypeFeedbackVector::kReservedIndexCount > 0);
> DCHECK(expr->CallFeedbackICSlot().Valid() || (
>        (feedback_slot_index == 0 && call_type == Call::POSSIBLY_EVAL_CALL));
> 
> ?

As discussed offline, it will be 0 but it is dependent on kReservedIndexCount
being 1. Updated the code to if else to make it more readable. Done.

https://codereview.chromium.org/2122183002/diff/80001/test/cctest/cctest.status
File test/cctest/cctest.status (right):

https://codereview.chromium.org/2122183002/diff/80001/test/cctest/cctest.stat...
test/cctest/cctest.status:152: 'test-feedback-vector/VectorCallICStates': [PASS,
NO_IGNITION],
On 2016/07/12 09:36:20, rmcilroy wrote:
> On 2016/07/12 09:01:34, mythria wrote:
> > This fails because it checks for type feedback for Array function. Since we
> plan
> > to do it in future, I just updated the comment to keep track of it.
> 
> As discussed offline, maybe we could split this into two tests (one for
> construct, one without.

Done.

https://codereview.chromium.org/2122183002/diff/80001/test/cctest/cctest.stat...
test/cctest/cctest.status:155: 'test-feedback-vector/VectorCallCounts': [PASS,
NO_IGNITION],
On 2016/07/12 09:36:20, rmcilroy wrote:
> On 2016/07/12 09:01:34, mythria wrote:
> > This fails because it also checks for type feedback from new along with
call.
> I
> > will address the type feedback for construct next. It should work then.
> 
> As discussed offline, maybe we could split this into two tests (one for
> construct, one without.

Done.

https://codereview.chromium.org/2122183002/diff/160001/test/cctest/test-feedb...
File test/cctest/test-feedback-vector.cc (right):

https://codereview.chromium.org/2122183002/diff/160001/test/cctest/test-feedb...
test/cctest/test-feedback-vector.cc:293:
CHECK(feedback_vector->Get(slot)->IsWeakCell());
This check was not there in the original test. I added this to be consistent
with other tests. They check if it is monomorphic and the call count. If it is
not required/safe to do it, I can drop this check.

[mythria, 2016-07-12 15:47:00]

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

[commit-bot: I haz the power, 2016-07-12 15:47: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-12 16:14:44]

The CQ bit was unchecked by commit-bot@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-12 16:14:45]

Dry run: This issue passed the CQ dry run.

[Benedikt Meurer, 2016-07-12 18:30:37]

Still LGTM. Thanks Mythri!

[mythria, 2016-07-13 07:56:00]

The CQ bit was checked by mythria@chromium.org

[mythria, 2016-07-13 07:56:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
yangguo@chromium.org, rmcilroy@chromium.org, mvstanton@chromium.org
Link to the patchset: https://codereview.chromium.org/2122183002/#ps190001
(title: "updated cctest.status to mark the tests fail with ignition.")

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

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-13 07:58:49]

Committed patchset #11 (id:190001)

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

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 08:00:34]

Description was changed from

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

Collect type feedback in the call bytecode handler. The current
implementation only collects feedback for JS function objects. The other
objects and Array functions do not collect any feedback. They will be
marked Megamorphic.

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

to

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

Collect type feedback in the call bytecode handler. The current
implementation only collects feedback for JS function objects. The other
objects and Array functions do not collect any feedback. They will be
marked Megamorphic.

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

Committed: https://crrev.com/fd420203ecb5abe427bda14aa896d90919d998fb
Cr-Commit-Position: refs/heads/master@{#37700}
==========

[commit-bot: I haz the power, 2016-07-13 08:00:35]

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