Replace ICStub for array.length with hydrogen stub

Replace ICStub for array.length with hydrogen stub


BUG=

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

[Dmitry Lomov (no reviews), 2013-03-11 08:45:00]

Replacing handwritten IC stubs for array.length with hydrogen-generated ones.
Still waiting for Golem job results, but take a look.

Dmitry

[Dmitry Lomov (no reviews), 2013-03-11 17:31:31]

Please take a look. I'll be looking at better hydrogen tomorrow. 

There is no platform-specific code (cool!), but there is still too much special
casing for arrays.

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

https://codereview.chromium.org/12700006/diff/7001/src/code-stubs.h#newcode618
src/code-stubs.h:618: }
Not entirely happy about this code; probably need to parametrize
HydrogenCodeStub with this

https://codereview.chromium.org/12700006/diff/7001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12700006/diff/7001/src/hydrogen.cc#newcode6750
src/hydrogen.cc:6750: 
Not sure if this is the best place yet to do this; the better way will be to do
the check in the polymorphic case and generate length load if all the maps are
array maps

[danno, 2013-03-12 11:16:16]

Some initial comments, I'll take another look when your polymorphic support
addresses the perf regressions.

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

https://codereview.chromium.org/12700006/diff/7001/src/code-stubs-hydrogen.cc...
src/code-stubs-hydrogen.cc:257: descriptor->register_param_count_ = 2;
Descriptors are persistent, but the lifetime of registers_ is not.

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

https://codereview.chromium.org/12700006/diff/7001/src/code-stubs.h#newcode618
src/code-stubs.h:618: }
Agreed. You shouldn't have to override FinishCode here.
On 2013/03/11 17:31:31, Dmitry Lomov (chromium) wrote:
> Not entirely happy about this code; probably need to parametrize
> HydrogenCodeStub with this

https://codereview.chromium.org/12700006/diff/7001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12700006/diff/7001/src/hydrogen.cc#newcode6750
src/hydrogen.cc:6750: 
Those are two separate issues. For monomorphic handling, this is not a bad
place. For polymorphic handling, you will have to change
HandlePolymorphicLoadNamedField as you describe.

On 2013/03/11 17:31:31, Dmitry Lomov (chromium) wrote:
> Not sure if this is the best place yet to do this; the better way will be to
do
> the check in the polymorphic case and generate length load if all the maps are
> array maps

https://codereview.chromium.org/12700006/diff/7001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/12700006/diff/7001/src/ic.cc#newcode922
src/ic.cc:922: if (object->IsJSArray() &&
Doesn't GetProperty already handle this (it should)?

https://codereview.chromium.org/12700006/diff/7001/src/ic.cc#newcode1210
src/ic.cc:1210: return isolate()->stub_cache()->ComputeLoadArrayLength(
why not just:

Property index = 
  PropertyIndex::NewHeaderIndex(JSArray::kLengthOffset / kPointerSize);
return isolate()->stub_cache()->ComputeLoadField(
  name, receiver, holder, index);

That obviates the need for all the new code in stub-cache.cc

https://codereview.chromium.org/12700006/diff/7001/src/objects.cc
File src/objects.cc (left):

https://codereview.chromium.org/12700006/diff/7001/src/objects.cc#oldcode8941
src/objects.cc:8941: ASSERT(code->kind() == Code::STUB);
Why did you have to remove this? Should still be valid...

https://codereview.chromium.org/12700006/diff/7001/src/stub-cache.cc
File src/stub-cache.cc (right):

https://codereview.chromium.org/12700006/diff/7001/src/stub-cache.cc#newcode236
src/stub-cache.cc:236: Handle<Code>
StubCache::ComputeLoadArrayLength(Handle<Name> name,
Instead of adding these, how about just make CompileLoadField smarter?

https://codereview.chromium.org/12700006/diff/7001/src/stub-cache.cc#newcode1475
src/stub-cache.cc:1475: LoadFieldStub stub(reg, field.is_inobject(holder),
field.translate(holder));
If you make a new method, GetLoadFieldHandler that takes the field and the name
and returns either a FastArrayLengthStub or a LoadFieldStub?

[Dmitry Lomov (no reviews), 2013-03-13 10:52:34]

PTAL

I completely removed (Fast)ArrayLengthStub - it is just a field load now. Also
added handling of map transitions for loads (the load stays monomorphic, with
the updated map check)

[danno, 2013-03-13 11:49:48]

https://codereview.chromium.org/12700006/diff/23001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12700006/diff/23001/src/hydrogen.cc#newcode6746
src/hydrogen.cc:6746: AddCheckMapsWithTransitions(object, map);
I think with your changes, you may not need WithTransitions here, because we
will already have calculated the set of maps that we need to handle, including
those with transitions.

https://codereview.chromium.org/12700006/diff/23001/src/hydrogen.cc#newcode6771
src/hydrogen.cc:6771: AddCheckMapsWithTransitions(object, map);
I don't think that we need "WithTransitions" here, either.

https://codereview.chromium.org/12700006/diff/23001/src/hydrogen.cc#newcode6782
src/hydrogen.cc:6782: AddCheckMapsWithTransitions(object, map);
... nor here.

https://codereview.chromium.org/12700006/diff/23001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/12700006/diff/23001/src/ic.cc#newcode1033
src/ic.cc:1033: bool is_map_transition = false;
How about refactoring this code into a method
"IsTransitionedMapOfMonomorphicTarget" on the IC? so the code at the bottom
would be
if (IsTransitionedMapOfMonomorphicTarget(receiver_map)) {
  UpdateMonomorphic...

https://codereview.chromium.org/12700006/diff/23001/src/ic.cc#newcode1171
src/ic.cc:1171: 
nit: remove extra whitespace

[Toon Verwaest, 2013-03-13 11:56:48]

https://codereview.chromium.org/12700006/diff/23001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/12700006/diff/23001/src/ic.cc#newcode1050
src/ic.cc:1050: : NULL;
I think we could make this more generic by following back pointers rather than
forward element transitions. This way all transitioning loads would benefit.

[danno, 2013-03-13 12:02:13]

I don't think we should follow them at all. Our mechanism to handle polymorphism
should get precisely the right set of maps after some amount of time, including
staying monomorphic if there are transitions (with this patch), so there is no
benefit to doing extra checks that we have no evidence will actually ever get
used.

[Dmitry Lomov (no reviews), 2013-03-13 16:14:31]

PTAL

https://codereview.chromium.org/12700006/diff/23001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12700006/diff/23001/src/hydrogen.cc#newcode6746
src/hydrogen.cc:6746: AddCheckMapsWithTransitions(object, map);
On 2013/03/13 11:49:48, danno wrote:
> I think with your changes, you may not need WithTransitions here, because we
> will already have calculated the set of maps that we need to handle, including
> those with transitions.

Done.

https://codereview.chromium.org/12700006/diff/23001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/12700006/diff/23001/src/ic.cc#newcode1033
src/ic.cc:1033: bool is_map_transition = false;
On 2013/03/13 11:49:48, danno wrote:
> How about refactoring this code into a method
> "IsTransitionedMapOfMonomorphicTarget" on the IC? so the code at the bottom
> would be
> if (IsTransitionedMapOfMonomorphicTarget(receiver_map)) {
>   UpdateMonomorphic...

Done.

https://codereview.chromium.org/12700006/diff/23001/src/ic.cc#newcode1171
src/ic.cc:1171: 
On 2013/03/13 11:49:48, danno wrote:
> nit: remove extra whitespace

Done.

[Dmitry Lomov (no reviews), 2013-03-19 13:32:12]

PTAL.
Added handling of polymorphic case, addressed performance regressions.

[danno, 2013-03-22 14:25:40]

lgtm with comments

https://codereview.chromium.org/12700006/diff/29002/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12700006/diff/29002/src/hydrogen.cc#newcode1055
src/hydrogen.cc:1055: 
nit: two spaces before function

https://codereview.chromium.org/12700006/diff/29002/src/hydrogen.cc#newcode6009
src/hydrogen.cc:6009: 
nit: two spaces after function

https://codereview.chromium.org/12700006/diff/29002/src/hydrogen.cc#newcode6118
src/hydrogen.cc:6118: }
nit: two spaces between functions

https://codereview.chromium.org/12700006/diff/29002/src/hydrogen.cc#newcode6138
src/hydrogen.cc:6138: }
nit: two spaces between functions.

https://codereview.chromium.org/12700006/diff/29002/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/12700006/diff/29002/src/ic.cc#newcode1014
src/ic.cc:1014: 
nit: two spaces before and after function

[Dmitry Lomov (no reviews), 2013-03-28 12:43:35]

Committed patchset #11 manually as r14090 (presubmit successful).