[stubs] Port KeyedLoadIC_Generic stub to TurboFan

[stubs] Port KeyedLoadIC_Generic stub to TurboFan

And also handle dictionary elements loads in the dispatcher.

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

[Jakob Kummerow, 2016-08-12 15:07:08]

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

[Jakob Kummerow, 2016-08-12 15:07:09]

Toon: PTAL.

Igor: FYI.

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

https://codereview.chromium.org/2245683002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:633: void
CodeStubAssembler::BranchIfPrototypesHaveNoElements(
This is pulled out from below (BranchIfFastJSArray).

[caitp, 2016-08-12 15:49:53]

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

https://codereview.chromium.org/2245683002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:676: // Bailout if receiver has slow elements.
The re-worded comments are appreciated :)

[Toon Verwaest, 2016-08-23 10:39:43]

Overall looks good, some comments.

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

https://codereview.chromium.org/2245683002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:3953: // Check that the receiver is some kind of JS
object but not a JSValue.
Seems like you want to check for LAST_CUSTOM_ELEMENTS_RECEIVER

https://codereview.chromium.org/2245683002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:3958: GotoIf(Int32LessThan(instance_type,
Int32Constant(JS_API_OBJECT_TYPE)),
You want JS_SPECIAL_API_OBJECT_TYPE I think, not JS_API_OBJECT_TYPE, which just
indicates that this regular JS_OBJECT was allocated for an API function but
doesn't need special handling.
If you check LAST_CUSTOM_ELEMENTS_RECEIVER above, that includes
JS_SPECIAL_API_OBJECT_TYPE, and will already exclude everything that will fail
EmitSpecialReceiverCheck.

https://codereview.chromium.org/2245683002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:3967: // TODO(jkummerow): Unify this with
CodeStubAssembler::TryToName().
Why not already do this? I'd prefer not have this semi-duplicated code here.

https://codereview.chromium.org/2245683002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:4078: GotoIf(Word32Equal(instance_type,
Int32Constant(JS_GLOBAL_OBJECT_TYPE)),
These types are already excluded with LAST_SPECIAL_RECEIVER_TYPE above (or
LAST_CUSTOM_ELEMENTS_RECEIVER if we decide to use that).

https://codereview.chromium.org/2245683002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:4080: GotoIf(Word32Equal(instance_type,
Int32Constant(JS_GLOBAL_PROXY_TYPE)),
This type will never have a property dictionary anyway; but it's also excluded.

[Jakob Kummerow, 2016-08-25 13:03:51]

Thanks for the review.

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

https://codereview.chromium.org/2245683002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:3953: // Check that the receiver is some kind of JS
object but not a JSValue.
On 2016/08/23 10:39:43, Toon Verwaest wrote:
> Seems like you want to check for LAST_CUSTOM_ELEMENTS_RECEIVER

Done.

https://codereview.chromium.org/2245683002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:3958: GotoIf(Int32LessThan(instance_type,
Int32Constant(JS_API_OBJECT_TYPE)),
On 2016/08/23 10:39:43, Toon Verwaest wrote:
> You want JS_SPECIAL_API_OBJECT_TYPE I think, not JS_API_OBJECT_TYPE, which
just
> indicates that this regular JS_OBJECT was allocated for an API function but
> doesn't need special handling.
> If you check LAST_CUSTOM_ELEMENTS_RECEIVER above, that includes
> JS_SPECIAL_API_OBJECT_TYPE, and will already exclude everything that will fail
> EmitSpecialReceiverCheck.

Done.

https://codereview.chromium.org/2245683002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:3967: // TODO(jkummerow): Unify this with
CodeStubAssembler::TryToName().
On 2016/08/23 10:39:43, Toon Verwaest wrote:
> Why not already do this? I'd prefer not have this semi-duplicated code here.

The current implementation is a faithful port of the handwritten stub's
behavior. TryToName is rather different in the details of its implementation
(doesn't handle heap numbers containing integers at all, checks other bits in
different order so e.g. doesn't try to load cached array indices from
non-internalized strings), so unifying the two would cause functional changes
for callers of each. I'd like to keep the performance and/or behavioral impact
of doing that isolated to a separate commit.
Are you OK with addressing this TODO as my very next CL?

https://codereview.chromium.org/2245683002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:4078: GotoIf(Word32Equal(instance_type,
Int32Constant(JS_GLOBAL_OBJECT_TYPE)),
On 2016/08/23 10:39:43, Toon Verwaest wrote:
> These types are already excluded with LAST_SPECIAL_RECEIVER_TYPE above (or
> LAST_CUSTOM_ELEMENTS_RECEIVER if we decide to use that).

Done.

https://codereview.chromium.org/2245683002/diff/1/src/code-stub-assembler.cc#...
src/code-stub-assembler.cc:4080: GotoIf(Word32Equal(instance_type,
Int32Constant(JS_GLOBAL_PROXY_TYPE)),
On 2016/08/23 10:39:43, Toon Verwaest wrote:
> This type will never have a property dictionary anyway; but it's also
excluded.

Done.

[Toon Verwaest, 2016-08-29 12:09:40]

lgtm

[Jakob Kummerow, 2016-08-29 12:37:56]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-08-29 12:37:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2245683002/#ps40001
(title: "rebased")

[commit-bot: I haz the power, 2016-08-29 12:38:06]

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-29 12:59:15]

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

[commit-bot: I haz the power, 2016-08-29 12:59:16]

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_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)
  v8_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/11762)
  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...)

[Jakob Kummerow, 2016-09-01 09:07:26]

The CQ bit was checked by jkummerow@chromium.org

[Jakob Kummerow, 2016-09-01 09:07:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
verwaest@chromium.org
Link to the patchset: https://codereview.chromium.org/2245683002/#ps60001
(title: "rebased past fixes")

[commit-bot: I haz the power, 2016-09-01 09:07:36]

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-01 09:39:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-01 09:39:25]

Description was changed from

==========
[stubs] Port KeyedLoadIC_Generic stub to TurboFan

And also handle dictionary elements loads in the dispatcher.
==========

to

==========
[stubs] Port KeyedLoadIC_Generic stub to TurboFan

And also handle dictionary elements loads in the dispatcher.

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

[commit-bot: I haz the power, 2016-09-01 09:39:26]

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

[Jakob Kummerow, 2016-09-01 16:06:57]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2303793002/ by jkummerow@chromium.org.

The reason for reverting is: Suspected to cause webgl_conformance_tests
failures:

WebglConformance_conformance_textures_misc_tex_image_with_format_and_type
(gpu_tests.webgl_conformance_integration_test.WebGLConformanceIntegrationTest)
... [21956:1299:0901/082859:INFO:CONSOLE(11)] "Uncaught TypeError: Cannot read
property 'charCodeAt' of undefined", source:  (11)
[21956:1299:0901/082859:INFO:CONSOLE(174)] "Uncaught TypeError: Cannot read
property 'charCodeAt' of undefined", source: http://127.0.0.1:56892/js/pnglib.js
(174).