Consolidated all the key store/load classes in the Hydrogen and Lithium

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

[mvstanton, 2012-10-20 00:43:03]

Hi Michael,
This changeset is actually pretty simple: eliminate the profusion of different
H<Load|Store>Keyed... classes, and the same for the Lithium classes. Caveats:

1) Nothing intelligent is happening regarding hoisting or recognizing the common
ElementKind in the lattice that should be shared by a phi. That should be after
this basic refactoring.
2) There are 2 failing mjsunit tests, related to the LoadKeyed changes, under
investigation.
3) A set of TODOs are sprinkled around representing questions. In parallel with
any of your comments I'm addressing those...any insight is helpful.
4) I can't build mips or mipsel (compile errors, even in master branch). Any
advice for this?

Thanks!
--Michael

[mvstanton, 2012-10-20 01:34:15]

Addressed the failing tests. I had introduced a logic error where we wouldn't
look for the hole in cases where we should.

[danno, 2012-10-21 20:44:41]

This is in general going in exactly the right direction. Biggest feedback:
is_external is redundant when you have elements_kind. There are also several
cases that can share even more code.

Please not that I just inspected and commented the ARM code, but I suspect my
comments apply to ia32 and x64, too.

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

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1875: if (instr->RequiresHoleCheck())
AssignEnvironment(result);
make sure that AssignEnvironment is the last thing done for consistency. It
should operate on the result of DefineAsRegister(result).

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1888: LOperand* key =
UseRegisterOrConstant(instr->key());
This can be UseRegisterOrConstantAtStart, I think, too, so you can combine the
key calculate. UseRegisterOrConstantAtStart just means that it can re-use the
register passed in for the key as the output register, if I recall correctly,
UseRegisterOrConstant doesn't. In the arm case (and other platforms, too), the
load result and the key aren't live at the same time for any of the load
variants, so it's OK to use UseRegisterOrConstantAtStart.

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1894: return (elements_kind ==
EXTERNAL_UNSIGNED_INT_ELEMENTS) ?
You can combine this exit sequence with the one for non external elements:

bool can_deoptimize = instr->RequiresHoleCheck() || elements_kin ==
EXTERNAL_UNSIGNED_INT_ELEMENTS;
return can_deoptimize ? AssignEnvironment(result) : result;

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1953: : UseRegister(instr->value());
Use the previous four lines to compute val in both the external and non-external
case. in the non external case, elements_kind won't be EXTERNAL_PIXEL_ELEMENTS
or EXTERNAL_FLOAT_ELEMENTS, so you'll get a temporary register as expected.

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1954: LOperand* key =
UseRegisterOrConstant(instr->key());
As in load case, use LOperand* key = UseRegisterOrConstantAtStart(instr->key());
for both branches.

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-ar...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:2999: 
nit: two line returns between functions

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:3006: bool key_is_constant =
instr->key()->IsConstantOperand();
Just for clarity and readability, perhaps it makes sense to factor this part of
the if info a helper function "DoLoadKeyedFixedDoubleArray", just like you did
for DoLoadKeyedExternal?

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:3044: Register result =
ToRegister(instr->result());
Perhaps here too with DoLoadKeyedFixedArray?

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:4068: 
nit: two spaces between functions

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:4074: DwVfpRegister value =
ToDoubleRegister(instr->value());
Same here as above, how about DoStoreKeyedFixedDoubleArray and
DoStoreKeyedFixedArray?

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....
src/hydrogen-instructions.cc:1855: stream->Add("[");
From the above line

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1861: }
To here can be shared at the end of the routine. RequiresHoleCheck should always
return false for external arrays, so it will work the way the external array
code expects.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1948: // fast double HLoadKeyed.
Yes, this should be OK

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:2014: stream->Add("[");
Can share from above line...

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:2017: value()->PrintNameTo(stream);
...to line above between both external and non-external cases.

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

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4258: if (!is_external) {
use IsExternalArrayElementsKind here

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4292: bool is_external() const { return
use IsExternalArrayElementsKind(elements_kind()) to calculate the result

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4345: // Is this nessecary at all?
We should be able to treat all array accesses uniformly, including optimizing
away bounds checks on external array, so this code shouldn't be necessary. BTW:
You should make sure that array bounds check elimination works for external
arrays after your changes.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4357: return is_external() ? true :
!RequiresHoleCheck();
RequiresHoleCheck should return false for external arrays, so IsDeletable should
just return !RequiresHoleCheck()

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4497: ElementsKind elements_kind, bool is_external)
Same feedback here about is_external as in the load case... it should be
computed from elements_kind

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4515: // (<tagged> | <external>>[int32] = <tagged> |
double | int
nit: (<tagged> | <external>)[int32] = (<tagged> | double | int)

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4523: ASSERT_EQ(index, 2);
Perhaps add a new predicate in elements-kind.h that detects all floating/double
types?

e.g. create:

inline bool IsDoubleOrFloatElementsKind(ElementsKind elements_kind) {
  return IsFastDoubleElementsKind(elements_kind) ||
   elements_kind == EXTERNAL_DOUBLE_ELEMENTS ||
   elements_kind == EXTERNAL_FLOAT_ELEMENTS;
}

then the rest of this function become:

if (IsDoubleOrFloatElementsKind()) return Representation::Double();
return is_external() ? Representation::Tagged() : Representation::Integer32();

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen.cc#newcode3749
src/hydrogen.cc:3749: // What should be done about that?
Why has the limitation changed?

[Sven Panne, 2012-10-22 06:14:18]

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....
src/hydrogen-instructions.cc:1865: switch (elements_kind()) {
DBC: Add a new function ElementsKind2String and use it in PrintElementsKind and
here.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:2021: switch (elements_kind()) {
See my comment above...

[mvstanton, 2012-10-23 23:44:20]

Hi all,
I responded to the great comments. Two questions remain:

1) I "stole" a bit from HLoadKeyed.indexoffsetvalue so that element_kind could
have 5 bits for it's representation. Any advice on consequences, tests, code to
visit for this? (my question about this is still present with a TODO(mvstanton)
in two code locations).

2) Danno, you mentioned that you had a wrong comment, related to
RequiresHoleCheck but I didn't quite understand.

Thanks,
--Michael

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

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1875: if (instr->RequiresHoleCheck())
AssignEnvironment(result);
On 2012/10/21 20:44:41, danno wrote:
> make sure that AssignEnvironment is the last thing done for consistency. It
> should operate on the result of DefineAsRegister(result).

Done.

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1888: LOperand* key =
UseRegisterOrConstant(instr->key());
On 2012/10/21 20:44:41, danno wrote:
> This can be UseRegisterOrConstantAtStart, I think, too, so you can combine the
> key calculate. UseRegisterOrConstantAtStart just means that it can re-use the
> register passed in for the key as the output register, if I recall correctly,
> UseRegisterOrConstant doesn't. In the arm case (and other platforms, too), the
> load result and the key aren't live at the same time for any of the load
> variants, so it's OK to use UseRegisterOrConstantAtStart.

Done.

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1894: return (elements_kind ==
EXTERNAL_UNSIGNED_INT_ELEMENTS) ?
On 2012/10/21 20:44:41, danno wrote:
> You can combine this exit sequence with the one for non external elements:
> 
> bool can_deoptimize = instr->RequiresHoleCheck() || elements_kin ==
> EXTERNAL_UNSIGNED_INT_ELEMENTS;
> return can_deoptimize ? AssignEnvironment(result) : result;

Thanks, done. Indeed I could combine a lot more code in there, such as the key
creation.

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1953: : UseRegister(instr->value());
On 2012/10/21 20:44:41, danno wrote:
> Use the previous four lines to compute val in both the external and
non-external
> case. in the non external case, elements_kind won't be EXTERNAL_PIXEL_ELEMENTS
> or EXTERNAL_FLOAT_ELEMENTS, so you'll get a temporary register as expected.

Done, but there was a pretty big variation in the ia32 tree that meant I
couldn't refactor as deeply there. Have a look at that change coming up and see
what you think.

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1954: LOperand* key =
UseRegisterOrConstant(instr->key());
On 2012/10/21 20:44:41, danno wrote:
> As in load case, use LOperand* key =
UseRegisterOrConstantAtStart(instr->key());
> for both branches.

Done.

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-ar...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:2999: 
On 2012/10/21 20:44:41, danno wrote:
> nit: two line returns between functions

Done.

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:3006: bool key_is_constant =
instr->key()->IsConstantOperand();
On 2012/10/21 20:44:41, danno wrote:
> Just for clarity and readability, perhaps it makes sense to factor this part
of
> the if info a helper function "DoLoadKeyedFixedDoubleArray", just like you did
> for DoLoadKeyedExternal?

Done.

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:4068: 
On 2012/10/21 20:44:41, danno wrote:
> nit: two spaces between functions

Done.

https://codereview.chromium.org/11238016/diff/2001/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:4074: DwVfpRegister value =
ToDoubleRegister(instr->value());
On 2012/10/21 20:44:41, danno wrote:
> Same here as above, how about DoStoreKeyedFixedDoubleArray and
> DoStoreKeyedFixedArray?

Done.

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....
src/hydrogen-instructions.cc:1855: stream->Add("[");
On 2012/10/21 20:44:41, danno wrote:
> From the above line

Done.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1861: }
On 2012/10/21 20:44:41, danno wrote:
> To here can be shared at the end of the routine. RequiresHoleCheck should
always
> return false for external arrays, so it will work the way the external array
> code expects.

Done.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1865: switch (elements_kind()) {
On 2012/10/22 06:14:18, Sven Panne wrote:
> DBC: Add a new function ElementsKind2String and use it in PrintElementsKind
and
> here.
Good idea, thanks Sven. Now, the string will be different (instead of "byte") it
would be EXTERNAL_BYTE_ELEMENTS, but I guess that is okay with us. I added an
assert to make sure that down this branch we only accept external_*
elementkinds.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:1948: // fast double HLoadKeyed.
On 2012/10/21 20:44:41, danno wrote:
> Yes, this should be OK

Done.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:2014: stream->Add("[");
On 2012/10/21 20:44:41, danno wrote:
> Can share from above line...

Done.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:2017: value()->PrintNameTo(stream);
On 2012/10/21 20:44:41, danno wrote:
> ...to line above between both external and non-external cases.

Done, also fixed duplicate code issue at line 2005.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.cc:2021: switch (elements_kind()) {
On 2012/10/22 06:14:18, Sven Panne wrote:
> See my comment above...

Done.

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

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4345: // Is this nessecary at all?
On 2012/10/21 20:44:41, danno wrote:
> We should be able to treat all array accesses uniformly, including optimizing
> away bounds checks on external array, so this code shouldn't be necessary.
BTW:
> You should make sure that array bounds check elimination works for external
> arrays after your changes.

Done.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4357: return is_external() ? true :
!RequiresHoleCheck();
On 2012/10/21 20:44:41, danno wrote:
> RequiresHoleCheck should return false for external arrays, so IsDeletable
should
> just return !RequiresHoleCheck()

Done.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4497: ElementsKind elements_kind, bool is_external)
On 2012/10/21 20:44:41, danno wrote:
> Same feedback here about is_external as in the load case... it should be
> computed from elements_kind

Done.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4515: // (<tagged> | <external>>[int32] = <tagged> |
double | int
On 2012/10/21 20:44:41, danno wrote:
> nit: (<tagged> | <external>)[int32] = (<tagged> | double | int)

Done.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen-instructions....
src/hydrogen-instructions.h:4523: ASSERT_EQ(index, 2);
On 2012/10/21 20:44:41, danno wrote:
> Perhaps add a new predicate in elements-kind.h that detects all
floating/double
> types?
> 
> e.g. create:
> 
> inline bool IsDoubleOrFloatElementsKind(ElementsKind elements_kind) {
>   return IsFastDoubleElementsKind(elements_kind) ||
>    elements_kind == EXTERNAL_DOUBLE_ELEMENTS ||
>    elements_kind == EXTERNAL_FLOAT_ELEMENTS;
> }
> 
> then the rest of this function become:
> 
> if (IsDoubleOrFloatElementsKind()) return Representation::Double();
> return is_external() ? Representation::Tagged() : Representation::Integer32();

Wow, that was quite a knot, thanks!

Actually, having spent an hour tracking down a bug, I found it here :D. Your
proposal should say
return is_external() ? Representation::INTEGER32() :   
Representation::TAGGED(); (swapped)

Damn, making me read instead of copying and pasting :D.

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/11238016/diff/2001/src/hydrogen.cc#newcode3749
src/hydrogen.cc:3749: // What should be done about that?
On 2012/10/21 20:44:41, danno wrote:
> Why has the limitation changed?

Because elements_kind was encoded in 5 bits, but because now it needs to hold
higher-numbered ElementsKinds it needs 6 bits. I'm not really seeing contracts
in the code around any particular #-of-bits for the index field. I'd be happy to
make some tests that make this more explicit. Any thoughts on that topic?

[danno, 2012-10-24 09:29:11]

>> you mentioned that you had a wrong comment, related to
RequiresHoleCheck but I didn't quite understand.

Going back to the original patch set, I couldn't find my comment. I must not
have saved it in the first place, so ignore everything I said about
RequiresHoleCheck! :-)

[danno, 2012-10-24 11:42:43]

Getting very close. Mostly nits.

Using the extra bit for ElementsKind is fine, by the way.

https://codereview.chromium.org/11238016/diff/4024/src/arm/lithium-codegen-ar...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/11238016/diff/4024/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:3001: void
LCodeGen::DoLoadKeyedDouble(LLoadKeyed* instr) {
nit: Just to make searches easier, maybe call this DoLoadKeyedFixedDouble. Even
better, call this DoLoadKeyedFixedDoubleArray and the other one
DoLoadKeyedFixedArray

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

https://codereview.chromium.org/11238016/diff/4024/src/hydrogen-instructions....
src/hydrogen-instructions.h:4285: SetGVNFlag(kDependsOnCalls);
Technically, I think the call to SetGVNFlag(kDependsOnCalls) isn't necessary,
but that's a deeper discussion outside the scope of this CL. Ask it about me
when you get back, it might be a second cleanup CL.

https://codereview.chromium.org/11238016/diff/4024/src/hydrogen-instructions....
src/hydrogen-instructions.h:4302: 
nit: remove whitespace change

https://codereview.chromium.org/11238016/diff/4024/src/hydrogen-instructions....
src/hydrogen-instructions.h:4349: // Is this okay?
That's fine. But crate a constant for the number of bits you use and add a
static assert here that makes sure that it is enough to hold ElementsKind, e.g.
STATIC_ASSERT(kElementsKindCount <= (1 << kBitsForElementsKind))

https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:2866: 
nit: two spaces

https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:3911: Register key = instr->key()->IsRegister()
? ToRegister(instr->key())
I think this still fits on one line?

https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-ia32.h#ne...
src/ia32/lithium-ia32.h:1395: LOperand* object() { return inputs_[0]; }
I think elements is actually better. No matter which variant, external or not,
this value is the "base" of the elements that you need to index into. Object has
a bit of a connotation that it's any HeapObject, or even a JavaScript object,
which it's not.

https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-ia32.h#ne...
src/ia32/lithium-ia32.h:1981: LOperand* object() { return inputs_[0]; }
Change this one to elements() for symmetry with LStoreKeyed.

https://codereview.chromium.org/11238016/diff/4024/src/x64/lithium-x64.h
File src/x64/lithium-x64.h (right):

https://codereview.chromium.org/11238016/diff/4024/src/x64/lithium-x64.h#newc...
src/x64/lithium-x64.h:1365: LOperand* object() { return inputs_[0]; }
elements?

[mvstanton, 2012-10-24 18:24:21]

Thanks for the comments Danno, here are my changes.

https://codereview.chromium.org/11238016/diff/4024/src/arm/lithium-codegen-ar...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/11238016/diff/4024/src/arm/lithium-codegen-ar...
src/arm/lithium-codegen-arm.cc:3001: void
LCodeGen::DoLoadKeyedDouble(LLoadKeyed* instr) {
On 2012/10/24 11:42:43, danno wrote:
> nit: Just to make searches easier, maybe call this DoLoadKeyedFixedDouble.
Even
> better, call this DoLoadKeyedFixedDoubleArray and the other one
> DoLoadKeyedFixedArray

Done.

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

https://codereview.chromium.org/11238016/diff/4024/src/hydrogen-instructions....
src/hydrogen-instructions.h:4285: SetGVNFlag(kDependsOnCalls);
On 2012/10/24 11:42:43, danno wrote:
> Technically, I think the call to SetGVNFlag(kDependsOnCalls) isn't necessary,
> but that's a deeper discussion outside the scope of this CL. Ask it about me
> when you get back, it might be a second cleanup CL.

okay, will do.

https://codereview.chromium.org/11238016/diff/4024/src/hydrogen-instructions....
src/hydrogen-instructions.h:4349: // Is this okay?
On 2012/10/24 11:42:43, danno wrote:
> That's fine. But crate a constant for the number of bits you use and add a
> static assert here that makes sure that it is enough to hold ElementsKind,
e.g.
> STATIC_ASSERT(kElementsKindCount <= (1 << kBitsForElementsKind)) 

Done.

https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-codegen-i...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:2866: 
On 2012/10/24 11:42:43, danno wrote:
> nit: two spaces

Done.

https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-codegen-i...
src/ia32/lithium-codegen-ia32.cc:3911: Register key = instr->key()->IsRegister()
? ToRegister(instr->key())
On 2012/10/24 11:42:43, danno wrote:
> I think this still fits on one line?

Done.

https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-ia32.h#ne...
src/ia32/lithium-ia32.h:1395: LOperand* object() { return inputs_[0]; }
On 2012/10/24 11:42:43, danno wrote:
> I think elements is actually better. No matter which variant, external or not,
> this value is the "base" of the elements that you need to index into. Object
has
> a bit of a connotation that it's any HeapObject, or even a JavaScript object,
> which it's not.

Done. And I pulled this all the way up into the hydrogen instructions too.
There, I remember we had elements() in the double case, but object() in the fast
case and external_pointer() in the external case. elements() does seem like the
best option.

https://codereview.chromium.org/11238016/diff/4024/src/ia32/lithium-ia32.h#ne...
src/ia32/lithium-ia32.h:1981: LOperand* object() { return inputs_[0]; }
On 2012/10/24 11:42:43, danno wrote:
> Change this one to elements() for symmetry with LStoreKeyed.

Done.

https://codereview.chromium.org/11238016/diff/4024/src/x64/lithium-x64.h
File src/x64/lithium-x64.h (right):

https://codereview.chromium.org/11238016/diff/4024/src/x64/lithium-x64.h#newc...
src/x64/lithium-x64.h:1365: LOperand* object() { return inputs_[0]; }
On 2012/10/24 11:42:43, danno wrote:
> elements?

Done.

[danno, 2012-10-29 14:56:13]

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):

http://codereview.chromium.org/11238016/diff/23001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1855: HLoadKeyed* instr) {
nit: this can be on the line before

http://codereview.chromium.org/11238016/diff/23001/src/arm/lithium-arm.cc#new...
src/arm/lithium-arm.cc:1903: HStoreKeyed* instr) {
nit: put on previous line

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
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.

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
nit: 
bool is_external() const {
  return ....;
}

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
total nit: M-Q is your emacs friend (reflow paragraph) :-)

http://codereview.chromium.org/11238016/diff/23001/src/hydrogen.cc#newcode3747
src/hydrogen.cc:3747: 
nit: Remove extraneous LF

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) {
nit: put on previous line

http://codereview.chromium.org/11238016/diff/23001/src/ia32/lithium-ia32.cc#n...
src/ia32/lithium-ia32.cc:1974: HStoreKeyed* instr) {
nit: put on previous line

[mvstanton, 2012-10-29 15:23:57]

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.