Chromium Code Reviews
Help | Chromium Project | Sign in
(504)

Issue 11238016: Consolidated all the key store/load classes in the Hydrogen and Lithium (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 6 months ago by mvstanton
Modified:
1 year, 5 months ago
CC:
v8-dev_googlegroups.com
Visibility:
Public.

Description

Consolidated all the key store/load classes in the Hydrogen and Lithium
space into just two:
HLoadKeyed/HLoadKeyedGeneric and HStoreKeyed/HStoreKeyedGeneric
LLoadKeyed/LLoadKeyedGeneric and LStoreKeyed/LStoreKeyedGeneric
BUG=

Committed: https://code.google.com/p/v8/source/detail?r=12839

Patch Set 1 #

Patch Set 2 : Fixed the 2 failing mjsunit tests #

Total comments: 47

Patch Set 3 : Respond to comments #

Patch Set 4 : Oops, missed a section of commented-out code #

Total comments: 17

Patch Set 5 : Response to comments from Danno #

Patch Set 6 : Oops, removed a TODO #

Total comments: 16

Patch Set 7 : Last batch of comment response, formatting issues. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1068 lines, -1619 lines) Lint Patch
M src/arm/lithium-arm.h View 1 2 3 4 5 chunks +19 lines, -101 lines 0 comments ? errors Download
M src/arm/lithium-arm.cc View 1 2 3 4 5 6 3 chunks +60 lines, -100 lines 0 comments ? errors Download
M src/arm/lithium-codegen-arm.h View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments ? errors Download
M src/arm/lithium-codegen-arm.cc View 1 2 3 4 5 chunks +232 lines, -213 lines 0 comments 0 errors Download
M src/elements-kind.h View 1 2 2 chunks +8 lines, -0 lines 0 comments ? errors Download
M src/elements-kind.cc View 1 2 1 chunk +7 lines, -2 lines 0 comments ? errors Download
M src/hydrogen.cc View 1 2 3 4 5 6 8 chunks +36 lines, -60 lines 0 comments ? errors Download
M src/hydrogen-instructions.h View 1 2 3 4 5 6 9 chunks +120 lines, -266 lines 0 comments ? errors Download
M src/hydrogen-instructions.cc View 1 2 3 4 5 6 8 chunks +41 lines, -134 lines 0 comments ? errors Download
M src/ia32/lithium-codegen-ia32.h View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments ? errors Download
M src/ia32/lithium-codegen-ia32.cc View 1 2 3 4 8 chunks +136 lines, -117 lines 0 comments ? errors Download
M src/ia32/lithium-ia32.h View 1 2 3 4 5 chunks +19 lines, -103 lines 0 comments ? errors Download
M src/ia32/lithium-ia32.cc View 1 2 3 4 5 6 3 chunks +78 lines, -114 lines 0 comments ? errors Download
M src/x64/lithium-codegen-x64.h View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments ? errors Download
M src/x64/lithium-codegen-x64.cc View 1 2 3 4 8 chunks +223 lines, -199 lines 0 comments ? errors Download
M src/x64/lithium-x64.h View 1 2 3 4 4 chunks +14 lines, -96 lines 0 comments ? errors Download
M src/x64/lithium-x64.cc View 1 2 3 4 5 6 3 chunks +57 lines, -114 lines 0 comments ? errors Download
Trybot results:
Commit:

Messages

Total messages: 10
mvstanton
Hi Michael, This changeset is actually pretty simple: eliminate the profusion of different H<Load|Store>Keyed... classes, ...
1 year, 6 months ago #1
mvstanton
Addressed the failing tests. I had introduced a logic error where we wouldn't look for ...
1 year, 6 months ago #2
danno
This is in general going in exactly the right direction. Biggest feedback: is_external is redundant ...
1 year, 6 months ago #3
Sven Panne
https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc File src/hydrogen-instructions.cc (right): https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions.cc#newcode1865 src/hydrogen-instructions.cc:1865: switch (elements_kind()) { DBC: Add a new function ElementsKind2String ...
1 year, 6 months ago #4
mvstanton
Hi all, I responded to the great comments. Two questions remain: 1) I "stole" a ...
1 year, 6 months ago #5
danno
>> you mentioned that you had a wrong comment, related to RequiresHoleCheck but I didn't ...
1 year, 6 months ago #6
danno
Getting very close. Mostly nits. Using the extra bit for ElementsKind is fine, by the ...
1 year, 6 months ago #7
mvstanton
Thanks for the comments Danno, here are my changes. https://codereview.chromium.org/11238016/diff/4024/src/arm/lithium-codegen-arm.cc File src/arm/lithium-codegen-arm.cc (right): https://codereview.chromium.org/11238016/diff/4024/src/arm/lithium-codegen-arm.cc#newcode3001 src/arm/lithium-codegen-arm.cc:3001: ...
1 year, 6 months ago #8
danno
LGTM with comments. Please wait to land this until bleeding_edge re-opens. http://codereview.chromium.org/11238016/diff/23001/src/arm/lithium-arm.cc File src/arm/lithium-arm.cc (right): ...
1 year, 5 months ago #9
mvstanton
1 year, 5 months ago #10
Thanks Danno. I've uploaded the last changeset and will await bleeding_edge
open.

http://codereview.chromium.org/11238016/diff/23001/src/arm/lithium-arm.cc
File src/arm/lithium-arm.cc (right):

http://codereview.chromium.org/11238016/diff/23001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1855: HLoadKeyed* instr) {
On 2012/10/29 14:56:13, danno wrote:
> nit: this can be on the line before

Done.

http://codereview.chromium.org/11238016/diff/23001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1903: HStoreKeyed* instr) {
On 2012/10/29 14:56:13, danno wrote:
> nit: put on previous line

Done.

http://codereview.chromium.org/11238016/diff/23001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

http://codereview.chromium.org/11238016/diff/23001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:2279: // If value was loaded from unboxed double
backing store or
On 2012/10/29 14:56:13, danno wrote:
> nit: fix the comment here, it's now out of date. You should explain that all
> keyed loads guarantee a non-hole value, so the canonicalization check isn't
> needed on stores that store a value taken from a keyed load.

Done.

http://codereview.chromium.org/11238016/diff/23001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

http://codereview.chromium.org/11238016/diff/23001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4291: bool is_external() const { return
On 2012/10/29 14:56:13, danno wrote:
> nit: 
> bool is_external() const {
>   return ....;
> }

Done.

http://codereview.chromium.org/11238016/diff/23001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/11238016/diff/23001/src/hydrogen.cc#newcode2721
src/hydrogen.cc:2721: // Storing a value into an external integer array is a
On 2012/10/29 14:56:13, danno wrote:
> total nit: M-Q is your emacs friend (reflow paragraph) :-)
Thanks!

http://codereview.chromium.org/11238016/diff/23001/src/hydrogen.cc#newcode3747
src/hydrogen.cc:3747: 
On 2012/10/29 14:56:13, danno wrote:
> nit: Remove extraneous LF

Done.

http://codereview.chromium.org/11238016/diff/23001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

http://codereview.chromium.org/11238016/diff/23001/src/ia32/lithium-ia32.cc#n...
src/ia32/lithium-ia32.cc:1927: HLoadKeyed* instr) {
On 2012/10/29 14:56:13, danno wrote:
> nit: put on previous line

Done.

http://codereview.chromium.org/11238016/diff/23001/src/ia32/lithium-ia32.cc#n...
src/ia32/lithium-ia32.cc:1974: HStoreKeyed* instr) {
On 2012/10/29 14:56:13, danno wrote:
> nit: put on previous line

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6