Reland 21774: Generate KeyedLoadGeneric with Hydrogen

Reland 21774: Generate KeyedLoadGeneric with Hydrogen

R=verwaest@chromium.org

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

[danno, 2013-11-29 11:32:28]

PTAL

[Toon Verwaest, 2013-12-04 17:29:26]

Added comments.

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1331: HValue* elements = AddLoadElements(receiver);
We need at least a BuildCheckHeapObject(receiver) before
AddLoadElements(receiver). Please add a test for this case.

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1349: KeyedLoadGenericElementStub* stub)
nit: Weird indentation

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1376: ElementsKind handle_as_kind) {
In isolation it's very hard to see what this method tries to accomplish. It's
very much tied to the way we efficiently check whether we can load a value from
the current array using a certain operation. At least add a comment to explain
this. (Essentially that the range [0, start-kind[ is already checked, and that
that's why we now implicitly check [start-kind, next_kind[ using bit_field2 <
next_kind).

I'd actually prefer to not pass in next_kind, but rather use LE on
handle_as_kind. (We'd probably have to ASSERT that handle_as_kind is next_kind -
1).

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1391: if
(!IsExternalArrayElementsKind(handle_as_kind)) {
It's hard to understand what kind of graph is created in case you have external
array elements. I'd prefer if you'd just make this a top-level if-test, rather
than selectively changing sub-instructions.

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1426: BuildReceiverCheck(receiver, bit_field_mask);
Can we pull these 2 checks out of the continuation? We do it in both branches.
Or is there a reason it needs to be done after BuildKeyedIndexCheck?

Also, will the checks IsAccessCheckNeeded / HasIndexedInterceptor ever change in
other places? If not, what about moving that into BuildReceiverCheck? And maybe
renaming "BuildReceiverCheck" to something that indicates that we're checking
whether the receiver is a "plain old javascript object" with safe access
semantics?

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs.h#newcod...
src/code-stubs.h:1886: virtual InlineCacheState GetICState() { return
MEGAMORPHIC; }
This should be GENERIC instead of MEGAMORPHIC.

https://codereview.chromium.org/57123002/diff/1270001/src/elements-kind.cc
File src/elements-kind.cc (right):

https://codereview.chromium.org/57123002/diff/1270001/src/elements-kind.cc#ne...
src/elements-kind.cc:70: ? 0 : (FixedArray::kHeaderSize - kSmiTagSize);
Shouldn't that be kHeapObjectTag rather than kSmiTagSize?
Also, this relies on FixedArray::kHeaderSize being equal to
FixedDoubleArray::kHeaderSize. Please add an ASSERT to that effect.

https://codereview.chromium.org/57123002/diff/1270001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/57123002/diff/1270001/src/heap.cc#newcode7715
src/heap.cc:7715: return -(index - map->inobject_properties() + 1);
Seems like this should be combined with the code in
PropertyIndex::translate/is_inobject. (Why is the index here and int rather than
a PropertyIndex object?)

https://codereview.chromium.org/57123002/diff/1270001/src/heap.cc#newcode7726
src/heap.cc:7726: return -index + map->inobject_properties() - 1;
Same as above.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-dehoist.cc
File src/hydrogen-dehoist.cc (right):

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-dehoist.cc...
src/hydrogen-dehoist.cc:56: if (value >= 1 <<
array_operation->MaxBaseOffsetBits() || value < 0) return;
"value >= 1 << ..." is quite hard to understand. Could we extract this to
temporary variables that declare their meaning?

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructio...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructio...
src/hydrogen-instructions.cc:3235: key_load->elements_kind(),
Why is this the same elements kind as the array of names? Wouldn't it make more
sense to know that the index cache is eg a FAST_SMI_ELEMENTS array, so that we
can avoid smi-checks? (For the future), even better, why don't we make the cache
external ints or so, so the HLoadFieldByIndex automatically gets the int without
any conversions?

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructio...
src/hydrogen-instructions.cc:3242: HChange::cast(int_index)->InsertBefore(this);
So this change is actually (or better, will be) superflous.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructio...
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructio...
src/hydrogen-instructions.h:6361: 
super nit: seems like we're pretty inconsistent in single or double newlines
between declarations in this patch of code. Can we just decide on something? :)

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructio...
src/hydrogen-instructions.h:6792: void IncreaseBaseOffset(uint32_t base_offset)
{
Could you add a comment indicating to what you are adding here
(constant-offset-from-key * kpointersize)?
And have a comment somewhere that indicates what the base-offset is made up off?
(header + constant-offset-from-key * kpointersize)

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1237: bit_field2, Add<HConstant>(Map::kElementsKindShift));
It seems like ElementsKind should really be a BitField of bitfield2. And then
perhaps we should have a function to read a BitField from an HValue; rather than
manually shifting and masking.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1465: Add<HLoadNamedField>(map,
HObjectAccess::ForMapBitField());
ForMapBitField2 I presume? Is this tested?
Seems to me like we should apply the same optimization here as above by swapping
the order of the bitfields. That would at least unify the code.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1471: access_check.If<HCompareNumericAndBranch>(and_result,
Use DeoptimizeIf.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1488: return AddUncasted<HShr>(index,
Add<HConstant>(String::kHashShift));
A bitfield decoding mechanism would be useful again.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1493: HIfContinuation* join_continuation) {
It took me a while to figure out why the branches below are ordered the way they
are. Please add a comment here indicating that join_continuation is basically
the continuation for if-true->smi, if-false->something-else; and that all
branches below need to match up to be able to join this continuation.
(Hence my confused comments below.)

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1510: IfBuilder not_string_or_name_if(this);
I'd swap the condition (use string_or_name rather than not_string_or_name). The
double negation in the else branch is a bit confusing.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1527: name_if.If<HCompareNumericAndBranch>(
And here name_if means that it's not a name. That's also pretty confusing.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1541: Add<HConstant>(Name::kContainsCachedArrayIndexMask));
Shouldn't kContainsCachedArrayIndex be on String rather than Name? Names can't
have cached array indices it seems to me.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1550: Push(BuildHashToIndex(hash));
BuildHashToIndex could just be a bitfield decoder.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1584: // not one of the global object types.
This comment seems to say the opposite of the name of the method and the actual
body. The code (and name) guarantees that the receiver *is* a global object, not
that it isn't.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1592: int globla_type_count = JS_GLOBAL_PROXY_TYPE -
JS_GLOBAL_OBJECT_TYPE;
globAL

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1607: builder.CaptureContinuation(continuation);
I wonder if the resulting code may be smaller / faster if you'd read out the
"is_dictionary_map" from the Map. That would avoid loading the HashTableMap.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.h#newcode1295
src/hydrogen.h:1295: HValue* BuildHashToIndex(HValue *hash);
nit: move * to HValue.

[danno, 2014-06-06 15:43:51]

Finally, after 7 months.... please take another look!

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1331: HValue* elements = AddLoadElements(receiver);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> We need at least a BuildCheckHeapObject(receiver) before
> AddLoadElements(receiver). Please add a test for this case.

Why is that needed? We already did a map check in the IC before we got here that
guaranteed that it's a JSObject with dictionary element kinds. I wrote a test
case for this an it works as expected (am I missing something?)

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1349: KeyedLoadGenericElementStub* stub)
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> nit: Weird indentation

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1376: ElementsKind handle_as_kind) {
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> In isolation it's very hard to see what this method tries to accomplish. It's
> very much tied to the way we efficiently check whether we can load a value
from
> the current array using a certain operation. At least add a comment to explain
> this. (Essentially that the range [0, start-kind[ is already checked, and that
> that's why we now implicitly check [start-kind, next_kind[ using bit_field2 <
> next_kind).
> 
> I'd actually prefer to not pass in next_kind, but rather use LE on
> handle_as_kind. (We'd probably have to ASSERT that handle_as_kind is next_kind
-
> 1).

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1391: if
(!IsExternalArrayElementsKind(handle_as_kind)) {
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> It's hard to understand what kind of graph is created in case you have
external
> array elements. I'd prefer if you'd just make this a top-level if-test, rather
> than selectively changing sub-instructions.

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1426: BuildReceiverCheck(receiver, bit_field_mask);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> Can we pull these 2 checks out of the continuation? We do it in both branches.
> Or is there a reason it needs to be done after BuildKeyedIndexCheck?
> 
> Also, will the checks IsAccessCheckNeeded / HasIndexedInterceptor ever change
in
> other places? If not, what about moving that into BuildReceiverCheck? And
maybe
> renaming "BuildReceiverCheck" to something that indicates that we're checking
> whether the receiver is a "plain old javascript object" with safe access
> semantics?

No, these can't be moved since they are different checks. This one here checks
to see if the object has indexed interceptors and the other path checks for
named interceptors.

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/57123002/diff/1270001/src/code-stubs.h#newcod...
src/code-stubs.h:1886: virtual InlineCacheState GetICState() { return
MEGAMORPHIC; }
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> This should be GENERIC instead of MEGAMORPHIC.

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/elements-kind.cc
File src/elements-kind.cc (right):

https://codereview.chromium.org/57123002/diff/1270001/src/elements-kind.cc#ne...
src/elements-kind.cc:70: ? 0 : (FixedArray::kHeaderSize - kSmiTagSize);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> Shouldn't that be kHeapObjectTag rather than kSmiTagSize?
> Also, this relies on FixedArray::kHeaderSize being equal to
> FixedDoubleArray::kHeaderSize. Please add an ASSERT to that effect.

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/57123002/diff/1270001/src/heap.cc#newcode7715
src/heap.cc:7715: return -(index - map->inobject_properties() + 1);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> Seems like this should be combined with the code in
> PropertyIndex::translate/is_inobject. (Why is the index here and int rather
than
> a PropertyIndex object?)

Well, essentially because they are are not the same as the index returned from
PropertyIndex.translate(), which returns negative values for in object
properties and positive values for out-of-object. Here, we need exactly the
opposite to work out-of-the-box with HLoadIndexField, which doesn't have a map
for the size of the object and therefore can use negative values to do the math
from the end of the object dynamically.

https://codereview.chromium.org/57123002/diff/1270001/src/heap.cc#newcode7726
src/heap.cc:7726: return -index + map->inobject_properties() - 1;
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> Same as above.

See my comment above.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-dehoist.cc
File src/hydrogen-dehoist.cc (right):

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-dehoist.cc...
src/hydrogen-dehoist.cc:56: if (value >= 1 <<
array_operation->MaxBaseOffsetBits() || value < 0) return;
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> "value >= 1 << ..." is quite hard to understand. Could we extract this to
> temporary variables that declare their meaning?

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructio...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructio...
src/hydrogen-instructions.cc:3235: key_load->elements_kind(),
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> Why is this the same elements kind as the array of names? Wouldn't it make
more
> sense to know that the index cache is eg a FAST_SMI_ELEMENTS array, so that we
> can avoid smi-checks? (For the future), even better, why don't we make the
cache
> external ints or so, so the HLoadFieldByIndex automatically gets the int
without
> any conversions?

That's beyond the scope of this change, which itself is no longer necessary
anyway after unrelated changes to HLoadFieldByIndex, the for-in case remains as
it always has been.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructio...
src/hydrogen-instructions.cc:3242: HChange::cast(int_index)->InsertBefore(this);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> So this change is actually (or better, will be) superflous.

This change is no longer necessary, after unrelated changes to
HLoadFieldByIndex, the for-in case remains as it always has been.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructio...
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructio...
src/hydrogen-instructions.h:6361: 
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> super nit: seems like we're pretty inconsistent in single or double newlines
> between declarations in this patch of code. Can we just decide on something?
:)

Well, a few weeks ago we decided that we would just defer to clang-format and be
done with it. I still think that's the right thing to do, I'll follow-up with
Jochen to see how far he got with that.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen-instructio...
src/hydrogen-instructions.h:6792: void IncreaseBaseOffset(uint32_t base_offset)
{
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> Could you add a comment indicating to what you are adding here
> (constant-offset-from-key * kpointersize)?
> And have a comment somewhere that indicates what the base-offset is made up
off?
> (header + constant-offset-from-key * kpointersize)

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1237: bit_field2, Add<HConstant>(Map::kElementsKindShift));
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> It seems like ElementsKind should really be a BitField of bitfield2. And then
> perhaps we should have a function to read a BitField from an HValue; rather
than
> manually shifting and masking.

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1465: Add<HLoadNamedField>(map,
HObjectAccess::ForMapBitField());
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> ForMapBitField2 I presume? Is this tested?
> Seems to me like we should apply the same optimization here as above by
swapping
> the order of the bitfields. That would at least unify the code.

Fields swapped, it makes this code much simpler.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1471: access_check.If<HCompareNumericAndBranch>(and_result,
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> Use DeoptimizeIf.

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1488: return AddUncasted<HShr>(index,
Add<HConstant>(String::kHashShift));
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> A bitfield decoding mechanism would be useful again.

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1493: HIfContinuation* join_continuation) {
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> It took me a while to figure out why the branches below are ordered the way
they
> are. Please add a comment here indicating that join_continuation is basically
> the continuation for if-true->smi, if-false->something-else; and that all
> branches below need to match up to be able to join this continuation.
> (Hence my confused comments below.)

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1510: IfBuilder not_string_or_name_if(this);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> I'd swap the condition (use string_or_name rather than not_string_or_name).
The
> double negation in the else branch is a bit confusing.

Ordering needs to stay the same due to continuation join.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1527: name_if.If<HCompareNumericAndBranch>(
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> And here name_if means that it's not a name. That's also pretty confusing.

Ordering needs to stay the same due to continuation join.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1541: Add<HConstant>(Name::kContainsCachedArrayIndexMask));
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> Shouldn't kContainsCachedArrayIndex be on String rather than Name? Names can't
> have cached array indices it seems to me.

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1550: Push(BuildHashToIndex(hash));
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> BuildHashToIndex could just be a bitfield decoder.

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1584: // not one of the global object types.
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> This comment seems to say the opposite of the name of the method and the
actual
> body. The code (and name) guarantees that the receiver *is* a global object,
not
> that it isn't.

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1592: int globla_type_count = JS_GLOBAL_PROXY_TYPE -
JS_GLOBAL_OBJECT_TYPE;
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> globAL

Done.

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1607: builder.CaptureContinuation(continuation);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> I wonder if the resulting code may be smaller / faster if you'd read out the
> "is_dictionary_map" from the Map. That would avoid loading the HashTableMap.

This is a copy of what the hand-written stub did IIRC. May I wait to try
optimizations in a separate CL?

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/57123002/diff/1270001/src/hydrogen.h#newcode1295
src/hydrogen.h:1295: HValue* BuildHashToIndex(HValue *hash);
On 2013/12/04 17:29:26, Toon Verwaest wrote:
> nit: move * to HValue.

Done.

[Toon Verwaest, 2014-06-10 11:33:44]

Mostly looks good. Some minor comments.

https://codereview.chromium.org/57123002/diff/1520001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/57123002/diff/1520001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1628: BuildReceiverCheck(receiver, bit_field_mask);
Any reason why you don't put this outside of the index_name_split, rather than
copying it in both branches?

https://codereview.chromium.org/57123002/diff/1520001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1670: Add<HConstant>(probe * 2));
* KeyedLookupCache::kEntryLength + KeyedLookupCache::kMapIndex ?

https://codereview.chromium.org/57123002/diff/1520001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1673: Add<HConstant>(probe * 2 + 1));
+ KeyedLookupCache::kKeyIndex

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1412: void HGraphBuilder::BuildReceiverCheck(HValue* receiver,
BuildCheckJSObject? You are obviously not allowing JSProxy as input, and in the
runtime JSReceiver is the superclass of JSProxy + everything JSObject.

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1487: // String: check whether the String is an String of an
index. If it is,
a String of an index

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1492: HValue* not_array_mask = Add<HConstant>(static_cast<int>(
I'd rather call this "not_index_mask".

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1495: HValue* not_array_test = AddUncasted<HBitwise>(
not_index_test

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1537: void HGraphBuilder::BuildGlobalInstanceTypeCheck(HValue*
receiver) {
I'd rename this to something like BuildCheckNonGlobalObject(HValue* receiver)

I don't think you want to include the JSGlobalProxy in this list. JSGlobalProxy
internally has fast properties I believe; but it doesn't matter, it never
actually has properties. It's not a ->IsGlobalobject(), which is only true for
the JSBuiltinsObject and JSGlobalobject. (The JSGlobalProxy in chrome has access
checks turned on anyway. So you won't even go down that path if the receiver is
the JSGlobalProxy.)

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1558: if_global_object.ThenDeopt("global instance type check
failed");
ThenDeopt("receiver was a global object"); ?

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1563: void HGraphBuilder::BuildCheckForDictionaryProperties(
Maybe don't name this CheckXXX? Check (at least to me) indicates that the
instruction deopts.

https://codereview.chromium.org/57123002/diff/1520001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/57123002/diff/1520001/src/objects.h#newcode9082
src/objects.h:9082: kArrayIndexValueBits> {};
Don't you need a // NOLINT here? Probably a good idea to run the presubmit
checks

https://codereview.chromium.org/57123002/diff/1520001/test/mjsunit/keyed-load...
File test/mjsunit/keyed-load-dictionary-stub.js (right):

https://codereview.chromium.org/57123002/diff/1520001/test/mjsunit/keyed-load...
test/mjsunit/keyed-load-dictionary-stub.js:14: return a[1];
I presume you wanted to write a[i] here?

[Toon Verwaest, 2014-06-10 11:37:07]

https://codereview.chromium.org/57123002/diff/1520001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/57123002/diff/1520001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1628: BuildReceiverCheck(receiver, bit_field_mask);
Aarg, you already indicated this in the comments on my previous review.
Nevermind ;)

On 2014/06/10 11:33:43, Toon Verwaest wrote:
> Any reason why you don't put this outside of the index_name_split, rather than
> copying it in both branches?

[danno, 2014-06-11 08:08:01]

Feedback addressed, please take another look.

https://codereview.chromium.org/57123002/diff/1520001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/57123002/diff/1520001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1670: Add<HConstant>(probe * 2));
On 2014/06/10 11:33:43, Toon Verwaest wrote:
> * KeyedLookupCache::kEntryLength + KeyedLookupCache::kMapIndex ?

Done.

https://codereview.chromium.org/57123002/diff/1520001/src/code-stubs-hydrogen...
src/code-stubs-hydrogen.cc:1673: Add<HConstant>(probe * 2 + 1));
On 2014/06/10 11:33:43, Toon Verwaest wrote:
> + KeyedLookupCache::kKeyIndex

Done.

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1412: void HGraphBuilder::BuildReceiverCheck(HValue* receiver,
On 2014/06/10 11:33:43, Toon Verwaest wrote:
> BuildCheckJSObject? You are obviously not allowing JSProxy as input, and in
the
> runtime JSReceiver is the superclass of JSProxy + everything JSObject.

Done.

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1487: // String: check whether the String is an String of an
index. If it is,
On 2014/06/10 11:33:43, Toon Verwaest wrote:
> a String of an index

Done.

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1492: HValue* not_array_mask = Add<HConstant>(static_cast<int>(
On 2014/06/10 11:33:43, Toon Verwaest wrote:
> I'd rather call this "not_index_mask".

Done.

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1495: HValue* not_array_test = AddUncasted<HBitwise>(
On 2014/06/10 11:33:43, Toon Verwaest wrote:
> not_index_test

Done.

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1537: void HGraphBuilder::BuildGlobalInstanceTypeCheck(HValue*
receiver) {
On 2014/06/10 11:33:44, Toon Verwaest wrote:
> I'd rename this to something like BuildCheckNonGlobalObject(HValue* receiver)
> 
> I don't think you want to include the JSGlobalProxy in this list.
JSGlobalProxy
> internally has fast properties I believe; but it doesn't matter, it never
> actually has properties. It's not a ->IsGlobalobject(), which is only true for
> the JSBuiltinsObject and JSGlobalobject. (The JSGlobalProxy in chrome has
access
> checks turned on anyway. So you won't even go down that path if the receiver
is
> the JSGlobalProxy.)

Done.

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1558: if_global_object.ThenDeopt("global instance type check
failed");
On 2014/06/10 11:33:43, Toon Verwaest wrote:
> ThenDeopt("receiver was a global object"); ?

Done.

https://codereview.chromium.org/57123002/diff/1520001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1563: void HGraphBuilder::BuildCheckForDictionaryProperties(
On 2014/06/10 11:33:43, Toon Verwaest wrote:
> Maybe don't name this CheckXXX? Check (at least to me) indicates that the
> instruction deopts.

Done.

https://codereview.chromium.org/57123002/diff/1520001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/57123002/diff/1520001/src/objects.h#newcode9082
src/objects.h:9082: kArrayIndexValueBits> {};
On 2014/06/10 11:33:44, Toon Verwaest wrote:
> Don't you need a // NOLINT here? Probably a good idea to run the presubmit
> checks

Done.

https://codereview.chromium.org/57123002/diff/1520001/test/mjsunit/keyed-load...
File test/mjsunit/keyed-load-dictionary-stub.js (right):

https://codereview.chromium.org/57123002/diff/1520001/test/mjsunit/keyed-load...
test/mjsunit/keyed-load-dictionary-stub.js:14: return a[1];
On 2014/06/10 11:33:44, Toon Verwaest wrote:
> I presume you wanted to write a[i] here?

Done.

[Toon Verwaest, 2014-06-11 11:27:59]

lgtm! (with 2 last nits)

https://codereview.chromium.org/57123002/diff/1540001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/57123002/diff/1540001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1425: Add<HLoadNamedField>(map, static_cast<HValue*>(NULL),
access);
nit: 4-space indent.

https://codereview.chromium.org/57123002/diff/1540001/src/hydrogen.cc#newcode...
src/hydrogen.cc:1546: STATIC_ASSERT(JS_GLOBAL_PROXY_TYPE ==
JS_BUILTINS_OBJECT_TYPE + 1);
This second ASSERT is now superfluous.

[danno, 2014-06-11 14:56:56]

Committed patchset #30 manually as r21774 (presubmit successful).

[danno, 2014-06-11 21:45:37]

Committed patchset #31 manually as r21781 (presubmit successful).