Use the property load IC for accessing the array length.

Use the property load IC for accessing the array length.

BUG=

[Jakob Kummerow, 2012-11-16 12:57:10]

https://chromiumcodereview.appspot.com/11299004/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/11299004/diff/1/src/hydrogen.cc#newcod...
src/hydrogen.cc:6669: fast_mode = false;
Why do you always set this to false? You could do the same trick as for
is_array: initialize to true, and inside the loop set to false if necessary. You
can even keep the existing "break;", just add the following after the if-block:

fast_mode = fast_mode && IsFastElementsKind(current_map->elements_kind());

https://chromiumcodereview.appspot.com/11299004/diff/1/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/11299004/diff/1/src/ic.cc#newcode987
src/ic.cc:987: // we have IC support only for getting the array length property.
nit: capital W

https://chromiumcodereview.appspot.com/11299004/diff/1/src/ic.cc#newcode988
src/ic.cc:988: if (holder->IsJSArray() &&
nit: You can avoid one level of indentation here by moving this up into a new
"else if" block, and keeping the original "else { ASSERT(...); return; }" after
that.

https://chromiumcodereview.appspot.com/11299004/diff/1/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/11299004/diff/1/src/objects.h#newcode8356
src/objects.h:8356: class PropertyIndex;
nit: two empty lines between top-level definitions (i.e. two before "class
PropertyIndex" and two after).

https://chromiumcodereview.appspot.com/11299004/diff/1/test/mjsunit/array-len...
File test/mjsunit/array-length.js (right):

https://chromiumcodereview.appspot.com/11299004/diff/1/test/mjsunit/array-len...
test/mjsunit/array-length.js:132: var getLength = function(a) { return a.length;
}
What's the point of doing this? Are you trying to get a fresh copy of the
function with uninitialized ICs? That won't work, because closures created from
the same literal will share the same code (it's why we have SharedFunctionInfo).

Otherwise, you could just declare these functions globally and avoid passing
them around all the time.

https://chromiumcodereview.appspot.com/11299004/diff/1/test/mjsunit/array-len...
test/mjsunit/array-length.js:147: length = 4;
unused variable?

[Massi, 2012-11-19 12:26:15]

I had to add changes to ignore element transitions in map check where
"appropriate" (where the check is guarding an array length access), this part of
the CL is new and should be reviewed as such.

I also addressed all code review comments (trivial changes).

https://chromiumcodereview.appspot.com/11299004/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/11299004/diff/1/src/hydrogen.cc#newcod...
src/hydrogen.cc:6669: fast_mode = false;
On 2012/11/16 12:57:10, Jakob wrote:
> Why do you always set this to false?

Because in this case I will emit an instance check instead of a map check, so I
really cannot know if the arrays that will be handled will be in fast more or
not.

https://chromiumcodereview.appspot.com/11299004/diff/1/src/ic.cc
File src/ic.cc (right):

https://chromiumcodereview.appspot.com/11299004/diff/1/src/ic.cc#newcode987
src/ic.cc:987: // we have IC support only for getting the array length property.
On 2012/11/16 12:57:10, Jakob wrote:
> nit: capital W

Done.

https://chromiumcodereview.appspot.com/11299004/diff/1/src/ic.cc#newcode988
src/ic.cc:988: if (holder->IsJSArray() &&
On 2012/11/16 12:57:10, Jakob wrote:
> nit: You can avoid one level of indentation here by moving this up into a new
> "else if" block, and keeping the original "else { ASSERT(...); return; }"
after
> that.

Done.

https://chromiumcodereview.appspot.com/11299004/diff/1/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/11299004/diff/1/src/objects.h#newcode8356
src/objects.h:8356: class PropertyIndex;
On 2012/11/16 12:57:10, Jakob wrote:
> nit: two empty lines between top-level definitions (i.e. two before "class
> PropertyIndex" and two after).

Done.

https://chromiumcodereview.appspot.com/11299004/diff/1/test/mjsunit/array-len...
File test/mjsunit/array-length.js (right):

https://chromiumcodereview.appspot.com/11299004/diff/1/test/mjsunit/array-len...
test/mjsunit/array-length.js:132: var getLength = function(a) { return a.length;
}
On 2012/11/16 12:57:10, Jakob wrote:
> What's the point of doing this? Are you trying to get a fresh copy of the
> function with uninitialized ICs?

Yes, now I am creating new functions every time.

https://chromiumcodereview.appspot.com/11299004/diff/1/test/mjsunit/array-len...
test/mjsunit/array-length.js:147: length = 4;
On 2012/11/16 12:57:10, Jakob wrote:
> unused variable?

No, I am setting the 'length property' of the global object.
Now I added a comment to state it :-)

[Jakob Kummerow, 2012-11-19 12:58:47]

LGTM with a nit.

https://chromiumcodereview.appspot.com/11299004/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/11299004/diff/1/src/hydrogen.cc#newcod...
src/hydrogen.cc:6669: fast_mode = false;
On 2012/11/19 12:26:15, Massi wrote:
> On 2012/11/16 12:57:10, Jakob wrote:
> > Why do you always set this to false?
> 
> Because in this case I will emit an instance check instead of a map check, so
I
> really cannot know if the arrays that will be handled will be in fast more or
> not.

Makes sense. Please add a comment to that effect. Suggestion:
        // We'll emit an instance type check when map_mode == false, which
        // doesn't check elements kinds, so we have to pessimistically assume
        // that some arrays will contain slow elements.

[Massi, 2012-11-21 10:26:17]

Addressed review comments and fixed a bug.

[Jakob Kummerow, 2012-11-21 12:13:39]

lgtm

[Jakob Kummerow, 2015-11-28 15:10:40]

Description was changed from

==========
Use the property load IC for accessing the array length.

BUG=
==========

to

==========
Use the property load IC for accessing the array length.

BUG=
==========