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

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

Created:
8 years, 2 months ago by mvstanton
Modified:
8 years, 1 month ago
CC:
v8-dev
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) Patch
M src/arm/lithium-arm.h View 1 2 3 4 5 chunks +19 lines, -101 lines 0 comments Download
M src/arm/lithium-arm.cc View 1 2 3 4 5 6 3 chunks +60 lines, -100 lines 0 comments Download
M src/arm/lithium-codegen-arm.h View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
M src/arm/lithium-codegen-arm.cc View 1 2 3 4 5 chunks +232 lines, -213 lines 0 comments Download
M src/elements-kind.h View 1 2 2 chunks +8 lines, -0 lines 0 comments Download
M src/elements-kind.cc View 1 2 1 chunk +7 lines, -2 lines 0 comments Download
M src/hydrogen.cc View 1 2 3 4 5 6 8 chunks +36 lines, -60 lines 0 comments Download
M src/hydrogen-instructions.h View 1 2 3 4 5 6 9 chunks +120 lines, -266 lines 0 comments Download
M src/hydrogen-instructions.cc View 1 2 3 4 5 6 8 chunks +41 lines, -134 lines 0 comments Download
M src/ia32/lithium-codegen-ia32.h View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
M src/ia32/lithium-codegen-ia32.cc View 1 2 3 4 8 chunks +136 lines, -117 lines 0 comments Download
M src/ia32/lithium-ia32.h View 1 2 3 4 5 chunks +19 lines, -103 lines 0 comments Download
M src/ia32/lithium-ia32.cc View 1 2 3 4 5 6 3 chunks +78 lines, -114 lines 0 comments Download
M src/x64/lithium-codegen-x64.h View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
M src/x64/lithium-codegen-x64.cc View 1 2 3 4 8 chunks +223 lines, -199 lines 0 comments Download
M src/x64/lithium-x64.h View 1 2 3 4 4 chunks +14 lines, -96 lines 0 comments Download
M src/x64/lithium-x64.cc View 1 2 3 4 5 6 3 chunks +57 lines, -114 lines 0 comments Download

Messages

Total messages: 10 (0 generated)
mvstanton
Hi Michael, This changeset is actually pretty simple: eliminate the profusion of different H<Load|Store>Keyed... classes, ...
8 years, 2 months ago (2012-10-20 00:43:03 UTC) #1
mvstanton
Addressed the failing tests. I had introduced a logic error where we wouldn't look for ...
8 years, 2 months ago (2012-10-20 01:34:15 UTC) #2
danno
This is in general going in exactly the right direction. Biggest feedback: is_external is redundant ...
8 years, 2 months ago (2012-10-21 20:44:41 UTC) #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 ...
8 years, 2 months ago (2012-10-22 06:14:18 UTC) #4
mvstanton
Hi all, I responded to the great comments. Two questions remain: 1) I "stole" a ...
8 years, 2 months ago (2012-10-23 23:44:20 UTC) #5
danno
>> you mentioned that you had a wrong comment, related to RequiresHoleCheck but I didn't ...
8 years, 2 months ago (2012-10-24 09:29:11 UTC) #6
danno
Getting very close. Mostly nits. Using the extra bit for ElementsKind is fine, by the ...
8 years, 2 months ago (2012-10-24 11:42:43 UTC) #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: ...
8 years, 2 months ago (2012-10-24 18:24:21 UTC) #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): ...
8 years, 1 month ago (2012-10-29 14:56:13 UTC) #9
mvstanton
8 years, 1 month ago (2012-10-29 15:23:57 UTC) #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.

Powered by Google App Engine
This is Rietveld 408576698