[ic] Support data handlers that represent loads from prototypes.

[ic] Support data handlers that represent loads from prototypes.

This CL also adds separate runtime call stats buckets for data handlers.

BUG=

Committed: https://crrev.com/73460009a6ef799d912f8ab7ebe38563d7242d03
Cr-Commit-Position: refs/heads/master@{#40281}

[Igor Sheludko, 2016-10-12 13:24:51]

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

[commit-bot: I haz the power, 2016-10-12 13:24: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-10-12 13:26:51]

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

[commit-bot: I haz the power, 2016-10-12 13:26:52]

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

[Igor Sheludko, 2016-10-12 13:36:46]

Patchset #1 (id:1) has been deleted

[Igor Sheludko, 2016-10-12 13:36:51]

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

[commit-bot: I haz the power, 2016-10-12 13:36:57]

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

[Igor Sheludko, 2016-10-12 13:42:48]

The CQ bit was unchecked by ishell@chromium.org

[Igor Sheludko, 2016-10-12 13:51:52]

Description was changed from

==========
[ic] Support non-code handlers that can handle loads from prototypes.

BUG=
==========

to

==========
[ic] Support non-code handlers that can represent loads from prototypes.

BUG=
==========

[Igor Sheludko, 2016-10-12 13:56:05]

Description was changed from

==========
[ic] Support non-code handlers that can represent loads from prototypes.

BUG=
==========

to

==========
[ic] Support non-code handlers that represent loads from prototypes.

BUG=
==========

[Igor Sheludko, 2016-10-12 14:10:12]

ishell@chromium.org changed reviewers:
+ jkummerow@chromium.org

[Igor Sheludko, 2016-10-12 14:10:12]

PTAL

[Igor Sheludko, 2016-10-12 14:44:22]

Description was changed from

==========
[ic] Support non-code handlers that represent loads from prototypes.

BUG=
==========

to

==========
[ic] Support data handlers that represent loads from prototypes.

BUG=
==========

[Jakob Kummerow, 2016-10-13 11:59:26]

LGTM with nits.

https://codereview.chromium.org/2419513002/diff/20001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2419513002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:4673: Node* handler = var_handler.value();
nit: This (and its two friends in 4770 and 4794) shadows an identically named
variable in the outer scope (see parameter in line 4654). Please avoid this,
either by renaming, or by s/Node* // here and below.

https://codereview.chromium.org/2419513002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:4781: Assert(WordNotEqual(holder,
IntPtrConstant(0)));
Why is it guaranteed that this WeakCell has not been cleared? Please add a
comment.

https://codereview.chromium.org/2419513002/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/2419513002/diff/20001/src/ic/ic.cc#newcode844
src/ic/ic.cc:844: if (!FLAG_tf_load_ic_stub) return Handle<Object>::null();
nit: you probably want to make the flag non-readonly again ;-)

https://codereview.chromium.org/2419513002/diff/20001/src/ic/ic.cc#newcode870
src/ic/ic.cc:870: JSObject* prototype =
JSObject::cast(current_map->prototype());
nit: use a PrototypeIterator please

https://codereview.chromium.org/2419513002/diff/20001/src/ic/ic.cc#newcode885
src/ic/ic.cc:885: return isolate()->factory()->NewTuple3(validity_cell,
holder_cell, handler);
nit: s/isolate()->factory()/factory/

https://codereview.chromium.org/2419513002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2419513002/diff/20001/src/objects.h#newcode6788
src/objects.h:6788: class Tuple3 : public Struct {
I would have called it Triple (and the values maybe x, y, z?), but Tuple3 is
fine too.

[Igor Sheludko, 2016-10-13 13:17:15]

Patchset #2 (id:40001) has been deleted

[Igor Sheludko, 2016-10-13 13:17:31]

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

[commit-bot: I haz the power, 2016-10-13 13:17:43]

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

[Igor Sheludko, 2016-10-13 13:20:38]

I addressed the comments. PTAL again

https://codereview.chromium.org/2419513002/diff/20001/src/code-stub-assembler.cc
File src/code-stub-assembler.cc (right):

https://codereview.chromium.org/2419513002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:4673: Node* handler = var_handler.value();
On 2016/10/13 11:59:26, Jakob Kummerow wrote:
> nit: This (and its two friends in 4770 and 4794) shadows an identically named
> variable in the outer scope (see parameter in line 4654). Please avoid this,
> either by renaming, or by s/Node* // here and below.

Done.

https://codereview.chromium.org/2419513002/diff/20001/src/code-stub-assembler...
src/code-stub-assembler.cc:4781: Assert(WordNotEqual(holder,
IntPtrConstant(0)));
On 2016/10/13 11:59:26, Jakob Kummerow wrote:
> Why is it guaranteed that this WeakCell has not been cleared? Please add a
> comment.

Done.

https://codereview.chromium.org/2419513002/diff/20001/src/ic/ic.cc
File src/ic/ic.cc (right):

https://codereview.chromium.org/2419513002/diff/20001/src/ic/ic.cc#newcode844
src/ic/ic.cc:844: if (!FLAG_tf_load_ic_stub) return Handle<Object>::null();
On 2016/10/13 11:59:26, Jakob Kummerow wrote:
> nit: you probably want to make the flag non-readonly again ;-)

We should just remove this flag once you remove the old keyed load IC
dispatcher.

https://codereview.chromium.org/2419513002/diff/20001/src/ic/ic.cc#newcode870
src/ic/ic.cc:870: JSObject* prototype =
JSObject::cast(current_map->prototype());
On 2016/10/13 11:59:26, Jakob Kummerow wrote:
> nit: use a PrototypeIterator please

Done.

https://codereview.chromium.org/2419513002/diff/20001/src/ic/ic.cc#newcode885
src/ic/ic.cc:885: return isolate()->factory()->NewTuple3(validity_cell,
holder_cell, handler);
On 2016/10/13 11:59:26, Jakob Kummerow wrote:
> nit: s/isolate()->factory()/factory/

Done.

https://codereview.chromium.org/2419513002/diff/20001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/2419513002/diff/20001/src/objects.h#newcode6788
src/objects.h:6788: class Tuple3 : public Struct {
On 2016/10/13 11:59:26, Jakob Kummerow wrote:
> I would have called it Triple (and the values maybe x, y, z?), but Tuple3 is
> fine too.

Let's keep it in a form TupleX because it scales (we will probably need Tuple4
for transitioning stores).

[commit-bot: I haz the power, 2016-10-13 13:24:54]

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

[commit-bot: I haz the power, 2016-10-13 13:24:54]

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/10742)

[Igor Sheludko, 2016-10-13 14:02:20]

Description was changed from

==========
[ic] Support data handlers that represent loads from prototypes.

BUG=
==========

to

==========
[ic] Support data handlers that represent loads from prototypes.

This CL also adds separate runtime call stats buckets for data handlers.

BUG=
==========

[Igor Sheludko, 2016-10-13 14:02:30]

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

[commit-bot: I haz the power, 2016-10-13 14:02:40]

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

[Jakob Kummerow, 2016-10-13 14:27:57]

The diff from patch set 1 -> 3 LGTM.

[Igor Sheludko, 2016-10-13 14:31:36]

The CQ bit was unchecked by ishell@chromium.org

[Igor Sheludko, 2016-10-13 14:31:41]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2016-10-13 14:31: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-10-13 15:04:03]

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

[commit-bot: I haz the power, 2016-10-13 15:04:04]

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

[epertoso, 2016-10-13 15:24:58]

epertoso@chromium.org changed reviewers:
+ epertoso@chromium.org

[epertoso, 2016-10-13 15:24:59]

lgtm for compiler

[Toon Verwaest, 2016-10-13 15:25:54]

verwaest@chromium.org changed reviewers:
+ verwaest@chromium.org

[Toon Verwaest, 2016-10-13 15:25:55]

(rubberstamp) lgtm

[Igor Sheludko, 2016-10-13 15:26:34]

The CQ bit was checked by ishell@chromium.org

[commit-bot: I haz the power, 2016-10-13 15:26:42]

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-10-13 16:18:59]

Description was changed from

==========
[ic] Support data handlers that represent loads from prototypes.

This CL also adds separate runtime call stats buckets for data handlers.

BUG=
==========

to

==========
[ic] Support data handlers that represent loads from prototypes.

This CL also adds separate runtime call stats buckets for data handlers.

BUG=
==========

[commit-bot: I haz the power, 2016-10-13 16:19:01]

Committed patchset #4 (id:100001)

[commit-bot: I haz the power, 2016-10-13 16:19:29]

Description was changed from

==========
[ic] Support data handlers that represent loads from prototypes.

This CL also adds separate runtime call stats buckets for data handlers.

BUG=
==========

to

==========
[ic] Support data handlers that represent loads from prototypes.

This CL also adds separate runtime call stats buckets for data handlers.

BUG=

Committed: https://crrev.com/73460009a6ef799d912f8ab7ebe38563d7242d03
Cr-Commit-Position: refs/heads/master@{#40281}
==========

[commit-bot: I haz the power, 2016-10-13 16:19:30]

Patchset 4 (id:??) landed as
https://crrev.com/73460009a6ef799d912f8ab7ebe38563d7242d03
Cr-Commit-Position: refs/heads/master@{#40281}