Introduce HTransitionElementsKind instruction.

Introduce HTransitionElementsKind instruction.


TEST=mjsunit/elements-kind

Committed: http://code.google.com/p/v8/source/detail?r=9702

[Jakob Kummerow, 2011-10-14 17:50:42]

PTAL.

Guidance:
hydrogen.cc, hydrogen-instructions.{h,cc}, lithium[-codegen]-ia32.{h,cc}: The
actual new instruction and its use.
runtime.{h,cc}, handles.{h,cc}, objects.{h,cc}: Two new runtime functions and
their implementation that do the heavy lifting.
builtins.{h,cc}, ic.h, ic-ia32.cc: Make said runtime functions callable from
optimized code.
ic.cc: just refactoring; the methods have been moved to objects.cc so that
hydrogen can use them.

This CL currently doesn't even compile for ARM and x64, I'll port it to those
architectures before landing.

[danno, 2011-10-17 12:36:05]

First round of feedback ia32 stuff applies to other platforms whee appropriate,
too.

http://codereview.chromium.org/8305001/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/8305001/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:3318: __ push(object_reg);
You always push object_reg, even if it isn't needed after the Transition. You
should have the register allocate help you more here. If you have object_reg as
a UseTempRegister _and_  TempRegister(edx), plus add a move between object_reg
and edx before the call, I think you get what you want and the register can do a
better job of rematerializing the value from the stack slot if it can. You
probably should ask Kevin if he knows some better trick.

http://codereview.chromium.org/8305001/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:3319: __ mov(new_map_reg, to_map);
You only need this is the FAST_SMI_ONLY_ELEMENTS -> FAST_DOUBLE_ELEMENTS and
FAST_DOUBLE_ELEMENTS -> FAST_ELEMENTS cases. Can you save the move in the
FAST_SMI_ONLY_ELEMENTS -> FAST_ELEMENTS case?

http://codereview.chromium.org/8305001/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:3346: __ pop(object_reg);
The register allocator should be able to help you here.

http://codereview.chromium.org/8305001/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8305001/diff/1/src/objects.cc#newcode2103
src/objects.cc:2103: return GetMapIfPresent(fast_map, candidates);
I think it might be clearer is you re-wrote GetMapIfPresent to be a "Contains"
predicate, so that this code reads like:

if (Contains(candidates, fast_map)) return fast_map;

http://codereview.chromium.org/8305001/diff/1/src/objects.cc#newcode2119
src/objects.cc:2119: if (fast_map_present != NULL) return fast_map_present;
fast_map_present is the same as fast_map, except and can be null, I think that
contains predicate would be clearer here, too.

http://codereview.chromium.org/8305001/diff/1/src/objects.cc#newcode9382
src/objects.cc:9382: }
This code should be unreachable, right? Perhaps an appropriate assert?

http://codereview.chromium.org/8305001/diff/1/src/objects.cc#newcode9383
src/objects.cc:9383: return Failure::Exception();
Is returning an exception failure really OK? Does that create the right
exception object to be thrown/etc? I think you need a DoThrow with the
appropriate exception object for this to be OK.

[Jakob Kummerow, 2011-10-18 08:13:09]

http://codereview.chromium.org/8305001/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/8305001/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:3318: __ push(object_reg);
On 2011/10/17 12:36:05, danno wrote:
> You always push object_reg, even if it isn't needed after the Transition. You
> should have the register allocate help you more here. If you have object_reg
as
> a UseTempRegister _and_  TempRegister(edx), plus add a move between object_reg
> and edx before the call, I think you get what you want and the register can do
a
> better job of rematerializing the value from the stack slot if it can. You
> probably should ask Kevin if he knows some better trick.

Done.

http://codereview.chromium.org/8305001/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:3319: __ mov(new_map_reg, to_map);
On 2011/10/17 12:36:05, danno wrote:
> You only need this is the FAST_SMI_ONLY_ELEMENTS -> FAST_DOUBLE_ELEMENTS and
> FAST_DOUBLE_ELEMENTS -> FAST_ELEMENTS cases. Can you save the move in the
> FAST_SMI_ONLY_ELEMENTS -> FAST_ELEMENTS case?

Nope, I really do need it in all cases (due to the write barrier, which expects
the written value in a register).

http://codereview.chromium.org/8305001/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:3346: __ pop(object_reg);
On 2011/10/17 12:36:05, danno wrote:
> The register allocator should be able to help you here.

Done.

http://codereview.chromium.org/8305001/diff/1/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8305001/diff/1/src/objects.cc#newcode2103
src/objects.cc:2103: return GetMapIfPresent(fast_map, candidates);
On 2011/10/17 12:36:05, danno wrote:
> I think it might be clearer is you re-wrote GetMapIfPresent to be a "Contains"
> predicate, so that this code reads like:
> 
> if (Contains(candidates, fast_map)) return fast_map;

Done.

http://codereview.chromium.org/8305001/diff/1/src/objects.cc#newcode2119
src/objects.cc:2119: if (fast_map_present != NULL) return fast_map_present;
On 2011/10/17 12:36:05, danno wrote:
> fast_map_present is the same as fast_map, except and can be null, I think that
> contains predicate would be clearer here, too.

Done.

http://codereview.chromium.org/8305001/diff/1/src/objects.cc#newcode9382
src/objects.cc:9382: }
On 2011/10/17 12:36:05, danno wrote:
> This code should be unreachable, right? Perhaps an appropriate assert?

This should indeed be unreachable, but we have mjsunit/fuzz-natives which calls
all runtime functions with default objects as arguments and expects us not to
crash.
I've moved the non-crashing check to runtime.cc and could then use an
UNREACHABLE() here.

http://codereview.chromium.org/8305001/diff/1/src/objects.cc#newcode9383
src/objects.cc:9383: return Failure::Exception();
On 2011/10/17 12:36:05, danno wrote:
> Is returning an exception failure really OK? Does that create the right
> exception object to be thrown/etc? I think you need a DoThrow with the
> appropriate exception object for this to be OK. 

Returning Failure::Exception is the way other methods in objects.cc that return
MaybeObject*s do it, and it seems to work just fine. But it's a moot point
either way, see comment above.

[danno, 2011-10-18 08:57:03]

LGTM with a few nits.

http://codereview.chromium.org/8305001/diff/1/src/ia32/lithium-codegen-ia32.cc
File src/ia32/lithium-codegen-ia32.cc (right):

http://codereview.chromium.org/8305001/diff/1/src/ia32/lithium-codegen-ia32.c...
src/ia32/lithium-codegen-ia32.cc:3318: __ push(object_reg);
On 2011/10/17 12:36:05, danno wrote:
> You always push object_reg, even if it isn't needed after the Transition. You
> should have the register allocate help you more here. If you have object_reg
as
> a UseTempRegister _and_  TempRegister(edx), plus add a move between object_reg
> and edx before the call, I think you get what you want and the register can do
a
> better job of rematerializing the value from the stack slot if it can. You
> probably should ask Kevin if he knows some better trick.

Done.

http://codereview.chromium.org/8305001/diff/6001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8305001/diff/6001/src/objects.cc#newcode2089
src/objects.cc:2089: bool Contains(MapList* maps_list, Map* map) {
nit: make static, rename to ContainsMap (Contains is too general).

http://codereview.chromium.org/8305001/diff/6001/src/objects.cc#newcode2103
src/objects.cc:2103: if (Contains(candidates, fast_map)) return fast_map;
return NULL here?

http://codereview.chromium.org/8305001/diff/6001/src/objects.cc#newcode9352
src/objects.cc:9352: FixedArray* elms = FixedArray::cast(elements());
Change the type to FixedArrayBase*, then you can hoist the capacity/length
calculatation before the ifs begin.

http://codereview.chromium.org/8305001/diff/6001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/8305001/diff/6001/src/runtime.cc#newcode4581
src/runtime.cc:4581: (from_kind == FAST_DOUBLE_ELEMENTS && to_kind ==
FAST_ELEMENTS)) {
Add a predicate IsValidFastElementTransition somewhere appropriate (static on
map?)

[Jakob Kummerow, 2011-10-18 12:05:15]

Nits fixed.
Landing this is on hold until Yang's CL with generated code for transitions on
ARM has landed, because tests depend on that.

http://codereview.chromium.org/8305001/diff/6001/src/objects.cc
File src/objects.cc (right):

http://codereview.chromium.org/8305001/diff/6001/src/objects.cc#newcode2089
src/objects.cc:2089: bool Contains(MapList* maps_list, Map* map) {
On 2011/10/18 08:57:03, danno wrote:
> nit: make static, rename to ContainsMap (Contains is too general).  

Done.
(For the record, I liked "Contains". It's a concrete implementation of a general
concept, and if someone defined a "Contains(FooList*, Foo*)" later, the compiler
would know what to do.)

http://codereview.chromium.org/8305001/diff/6001/src/objects.cc#newcode2103
src/objects.cc:2103: if (Contains(candidates, fast_map)) return fast_map;
On 2011/10/18 08:57:03, danno wrote:
> return NULL here?

Done.

http://codereview.chromium.org/8305001/diff/6001/src/objects.cc#newcode9352
src/objects.cc:9352: FixedArray* elms = FixedArray::cast(elements());
On 2011/10/18 08:57:03, danno wrote:
> Change the type to FixedArrayBase*, then you can hoist the capacity/length
> calculatation before the ifs begin.

Done. Good idea!

http://codereview.chromium.org/8305001/diff/6001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/8305001/diff/6001/src/runtime.cc#newcode4581
src/runtime.cc:4581: (from_kind == FAST_DOUBLE_ELEMENTS && to_kind ==
FAST_ELEMENTS)) {
On 2011/10/18 08:57:03, danno wrote:
> Add a predicate IsValidFastElementTransition somewhere appropriate (static on
> map?)

Done.