Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(897)

Issue 12700006: Replace ICStub for array.length with hydrogen stub (Closed)

Created:
7 years, 9 months ago by Dmitry Lomov (no reviews)
Modified:
7 years, 9 months ago
Reviewers:
danno, Toon Verwaest
CC:
v8-dev
Visibility:
Public.

Description

Replace ICStub for array.length with hydrogen stub BUG= Committed: https://code.google.com/p/v8/source/detail?r=14090

Patch Set 1 #

Patch Set 2 : Style #

Patch Set 3 : Proper rebase #

Patch Set 4 : New patch #

Total comments: 10

Patch Set 5 : Reworked to remove ArrayLengthStub completely #

Patch Set 6 : Removed dead declaration #

Patch Set 7 : More dead code #

Patch Set 8 : Even more dead code #

Total comments: 9

Patch Set 9 : CR feedback #

Patch Set 10 : Fixed performance #

Patch Set 11 : Removed debug print #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+90 lines, -166 lines) Patch
M src/arm/code-stubs-arm.cc View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -29 lines 0 comments Download
M src/ast.h View 1 2 3 4 5 6 7 3 chunks +0 lines, -3 lines 0 comments Download
M src/ast.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -4 lines 0 comments Download
M src/code-stubs.h View 1 2 3 4 5 6 7 8 9 2 chunks +0 lines, -11 lines 0 comments Download
M src/hydrogen.h View 1 2 3 4 5 6 7 8 9 3 chunks +8 lines, -0 lines 0 comments Download
M src/hydrogen.cc View 1 2 3 4 5 6 7 8 9 8 chunks +48 lines, -11 lines 4 comments Download
M src/ia32/code-stubs-ia32.cc View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -19 lines 0 comments Download
M src/ic.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M src/ic.cc View 1 2 3 4 5 6 7 8 10 5 chunks +32 lines, -32 lines 1 comment Download
M src/mips/code-stubs-mips.cc View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -29 lines 0 comments Download
M src/x64/code-stubs-x64.cc View 1 2 3 4 5 6 7 8 9 1 chunk +0 lines, -28 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
Dmitry Lomov (no reviews)
Replacing handwritten IC stubs for array.length with hydrogen-generated ones. Still waiting for Golem job results, ...
7 years, 9 months ago (2013-03-11 08:45:00 UTC) #1
Dmitry Lomov (no reviews)
Please take a look. I'll be looking at better hydrogen tomorrow. There is no platform-specific ...
7 years, 9 months ago (2013-03-11 17:31:31 UTC) #2
danno
Some initial comments, I'll take another look when your polymorphic support addresses the perf regressions. ...
7 years, 9 months ago (2013-03-12 11:16:16 UTC) #3
Dmitry Lomov (no reviews)
PTAL I completely removed (Fast)ArrayLengthStub - it is just a field load now. Also added ...
7 years, 9 months ago (2013-03-13 10:52:34 UTC) #4
danno
https://codereview.chromium.org/12700006/diff/23001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/12700006/diff/23001/src/hydrogen.cc#newcode6746 src/hydrogen.cc:6746: AddCheckMapsWithTransitions(object, map); I think with your changes, you may ...
7 years, 9 months ago (2013-03-13 11:49:48 UTC) #5
Toon Verwaest
https://codereview.chromium.org/12700006/diff/23001/src/ic.cc File src/ic.cc (right): https://codereview.chromium.org/12700006/diff/23001/src/ic.cc#newcode1050 src/ic.cc:1050: : NULL; I think we could make this more ...
7 years, 9 months ago (2013-03-13 11:56:48 UTC) #6
danno
I don't think we should follow them at all. Our mechanism to handle polymorphism should ...
7 years, 9 months ago (2013-03-13 12:02:13 UTC) #7
Dmitry Lomov (no reviews)
PTAL https://codereview.chromium.org/12700006/diff/23001/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/12700006/diff/23001/src/hydrogen.cc#newcode6746 src/hydrogen.cc:6746: AddCheckMapsWithTransitions(object, map); On 2013/03/13 11:49:48, danno wrote: > ...
7 years, 9 months ago (2013-03-13 16:14:31 UTC) #8
Dmitry Lomov (no reviews)
PTAL. Added handling of polymorphic case, addressed performance regressions.
7 years, 9 months ago (2013-03-19 13:32:12 UTC) #9
danno
lgtm with comments https://codereview.chromium.org/12700006/diff/29002/src/hydrogen.cc File src/hydrogen.cc (right): https://codereview.chromium.org/12700006/diff/29002/src/hydrogen.cc#newcode1055 src/hydrogen.cc:1055: nit: two spaces before function https://codereview.chromium.org/12700006/diff/29002/src/hydrogen.cc#newcode6009 ...
7 years, 9 months ago (2013-03-22 14:25:40 UTC) #10
Dmitry Lomov (no reviews)
7 years, 9 months ago (2013-03-28 12:43:35 UTC) #11
Message was sent while issue was closed.
Committed patchset #11 manually as r14090 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698