Generate the TransitionElementsStub using Crankshaft

Generate the TransitionElementsStub using Crankshaft

This includes:
* Adding support for saving callee-clobbered double registers in Crankshaft code.
* Adding a new "HTrapAllocationMemento" hydrogen instruction to handle AllocationSiteInfo data in crankshafted stubs.
* Adding a new "HAllocate" hydrogen instruction that can allocate raw memory from the GC in crankshafted code.
* Support for manipulation of the hole in HChange instructions for Crankshafted stubs.
* Utility routines to manually build loops and if statements containing hydrogen code.

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

[danno, 2013-01-30 11:47:13]

PTAL

[Michael Starzinger, 2013-01-31 13:15:09]

First round of comments, from a high-level point of view it looks good already.
I like this change. Just reviewed ia32 and the platform-independent stuff so
far.

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/heap.h
File src/heap.h (right):

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/heap.h#newcode229
src/heap.h:229: V(fixed_array_length_field_symbol, "%length")                   
      \
Let's just call this length_field_symbol, the symbol is agnostic to the type it
is used in.

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen-instr...
File src/hydrogen-instructions.h (right):

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen-instr...
src/hydrogen-instructions.h:4132: return (!object->IsAllocateObject() &&
!object->IsAllocate() &&
This only holds if HAllocate guarantees that it allocates in new-space. Since
HAllocate will be flexible enough to also allocate in old-space, we need an
additional predicate like HAllocate::GuaranteedInNewSpace() that we check here.

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen-instr...
src/hydrogen-instructions.h:5116: CAN_ALLOCATE_IN_OLD_SPACE = 1 << 1,
The old-space flag will need to indicate which old-space is targeted (i.e.
old-data-space, old-pointer-space). Allocation in LO-space is trickier, because
you cannot do the bulk-allocation trick there.

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.cc#ne...
src/hydrogen.cc:985:
store_map->SetSideEffectDominator(kChangesNewSpacePromotion, elements);
Since we are doing GVN we shouldn't need to set the dominator manually, let's
drop this.

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.cc#ne...
src/hydrogen.cc:994:
store_length->SetSideEffectDominator(kChangesNewSpacePromotion, elements);
Likewise.

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.h
File src/hydrogen.h (right):

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.h#new...
src/hydrogen.h:854: class LoopBodyBuilder {
The LoopBodyBuilder seems to be left-over, drop it.

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.h#new...
src/hydrogen.h:860: class CopyElementLoopBodyBuilder;
The CopyElementLoopBodyBuilder seems to be a left-over, drop it.

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.h#new...
src/hydrogen.h:916: HValue* IntegerConstant(int value);
There already is a HGraph::GetConstantInt32(), we could reuse that for the
well-known constants "0" and "1". It makes sure constants are inserted at the
top of the graph.

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/hydrogen.h#new...
src/hydrogen.h:995: friend class CopyElementLoopBodyBuilder;
The CopyElementLoopBodyBuilder seems to be a left-over, drop it.

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/ia32/codegen-i...
File src/ia32/codegen-ia32.cc (right):

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/ia32/codegen-i...
src/ia32/codegen-ia32.cc:404: masm->TestJSArrayForAllocationSiteInfo(edx, edi);
I like this change. But use "__" instead of "masm->" here. And in the two
occurrences below.

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/ia32/lithium-i...
File src/ia32/lithium-ia32.cc (right):

https://chromiumcodereview.appspot.com/11659022/diff/48001/src/ia32/lithium-i...
src/ia32/lithium-ia32.cc:2267: LOperand* context = UseAny(instr->context());
It seems info()->MarkAsDeferredCalling() is missing for LAllocateObject.

https://chromiumcodereview.appspot.com/11659022/diff/48001/test/mjsunit/regre...
File test/mjsunit/regress/generated-transition-stub.js (right):

https://chromiumcodereview.appspot.com/11659022/diff/48001/test/mjsunit/regre...
test/mjsunit/regress/generated-transition-stub.js:1: // Copyright 2013 the V8
project authors. All rights reserved.
Not really a regression test, let's move it to either test/mjsunit or
test/mjsunit/compiler.

[danno, 2013-01-31 15:53:25]

Please take another look. Note that I added a flag to disable the new
functionality.

https://codereview.chromium.org/11659022/diff/48001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/11659022/diff/48001/src/heap.h#newcode229
src/heap.h:229: V(fixed_array_length_field_symbol, "%length")                   
      \
On 2013/01/31 13:15:10, Michael Starzinger wrote:
> Let's just call this length_field_symbol, the symbol is agnostic to the type
it
> is used in.

Done.

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

https://codereview.chromium.org/11659022/diff/48001/src/hydrogen-instructions...
src/hydrogen-instructions.h:4132: return (!object->IsAllocateObject() &&
!object->IsAllocate() &&
On 2013/01/31 13:15:10, Michael Starzinger wrote:
> This only holds if HAllocate guarantees that it allocates in new-space. Since
> HAllocate will be flexible enough to also allocate in old-space, we need an
> additional predicate like HAllocate::GuaranteedInNewSpace() that we check
here.

Done.

https://codereview.chromium.org/11659022/diff/48001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5116: CAN_ALLOCATE_IN_OLD_SPACE = 1 << 1,
On 2013/01/31 13:15:10, Michael Starzinger wrote:
> The old-space flag will need to indicate which old-space is targeted (i.e.
> old-data-space, old-pointer-space). Allocation in LO-space is trickier,
because
> you cannot do the bulk-allocation trick there.

Done.

https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.cc#newcode985
src/hydrogen.cc:985:
store_map->SetSideEffectDominator(kChangesNewSpacePromotion, elements);
On 2013/01/31 13:15:10, Michael Starzinger wrote:
> Since we are doing GVN we shouldn't need to set the dominator manually, let's
> drop this.

Done.

https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.cc#newcode994
src/hydrogen.cc:994:
store_length->SetSideEffectDominator(kChangesNewSpacePromotion, elements);
On 2013/01/31 13:15:10, Michael Starzinger wrote:
> Likewise.

Done.

https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.h#newcode854
src/hydrogen.h:854: class LoopBodyBuilder {
On 2013/01/31 13:15:10, Michael Starzinger wrote:
> The LoopBodyBuilder seems to be left-over, drop it.

Done.

https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.h#newcode860
src/hydrogen.h:860: class CopyElementLoopBodyBuilder;
On 2013/01/31 13:15:10, Michael Starzinger wrote:
> The CopyElementLoopBodyBuilder seems to be a left-over, drop it.

Done.

https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.h#newcode860
src/hydrogen.h:860: class CopyElementLoopBodyBuilder;
On 2013/01/31 13:15:10, Michael Starzinger wrote:
> The CopyElementLoopBodyBuilder seems to be a left-over, drop it.

Done.

https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.h#newcode916
src/hydrogen.h:916: HValue* IntegerConstant(int value);
On 2013/01/31 13:15:10, Michael Starzinger wrote:
> There already is a HGraph::GetConstantInt32(), we could reuse that for the
> well-known constants "0" and "1". It makes sure constants are inserted at the
> top of the graph.

Done.

https://codereview.chromium.org/11659022/diff/48001/src/hydrogen.h#newcode995
src/hydrogen.h:995: friend class CopyElementLoopBodyBuilder;
On 2013/01/31 13:15:10, Michael Starzinger wrote:
> The CopyElementLoopBodyBuilder seems to be a left-over, drop it.

Done.

https://codereview.chromium.org/11659022/diff/48001/src/ia32/codegen-ia32.cc
File src/ia32/codegen-ia32.cc (right):

https://codereview.chromium.org/11659022/diff/48001/src/ia32/codegen-ia32.cc#...
src/ia32/codegen-ia32.cc:404: masm->TestJSArrayForAllocationSiteInfo(edx, edi);
On 2013/01/31 13:15:10, Michael Starzinger wrote:
> I like this change. But use "__" instead of "masm->" here. And in the two
> occurrences below.

Done.

https://codereview.chromium.org/11659022/diff/48001/src/ia32/lithium-ia32.cc
File src/ia32/lithium-ia32.cc (right):

https://codereview.chromium.org/11659022/diff/48001/src/ia32/lithium-ia32.cc#...
src/ia32/lithium-ia32.cc:2267: LOperand* context = UseAny(instr->context());
On 2013/01/31 13:15:10, Michael Starzinger wrote:
> It seems info()->MarkAsDeferredCalling() is missing for LAllocateObject.

Done.

[Rodolph Perfetta, 2013-01-31 20:05:55]

Drive by comments.

https://codereview.chromium.org/11659022/diff/50002/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/11659022/diff/50002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4579: if (!object_reg.is(r0)) {
Move(r0, object_reg);

https://codereview.chromium.org/11659022/diff/50002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4982: no_reg, vs);
Did you intend to make this vmov conditional?

https://codereview.chromium.org/11659022/diff/50002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:5125: no_reg, vs);
same here, did you mean vmovvs?

[danno, 2013-02-01 11:37:35]

Addressed feedback and fixed deoptimization problem with double values in
registers.

https://codereview.chromium.org/11659022/diff/50002/src/arm/lithium-codegen-a...
File src/arm/lithium-codegen-arm.cc (right):

https://codereview.chromium.org/11659022/diff/50002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4579: if (!object_reg.is(r0)) {
On 2013/01/31 20:05:55, Rodolph Perfetta wrote:
> Move(r0, object_reg);

Done.

https://codereview.chromium.org/11659022/diff/50002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4579: if (!object_reg.is(r0)) {
On 2013/01/31 20:05:55, Rodolph Perfetta wrote:
> Move(r0, object_reg);

Done.

https://codereview.chromium.org/11659022/diff/50002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4982: no_reg, vs);
On 2013/01/31 20:05:55, Rodolph Perfetta wrote:
> Did you intend to make this vmov conditional?

Done.

https://codereview.chromium.org/11659022/diff/50002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:4982: no_reg, vs);
On 2013/01/31 20:05:55, Rodolph Perfetta wrote:
> Did you intend to make this vmov conditional?

Done.

https://codereview.chromium.org/11659022/diff/50002/src/arm/lithium-codegen-a...
src/arm/lithium-codegen-arm.cc:5125: no_reg, vs);
On 2013/01/31 20:05:55, Rodolph Perfetta wrote:
> same here, did you mean vmovvs?

Done.

[Michael Starzinger, 2013-02-01 13:15:08]

This should be the last round of comments.

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2126: bool HLoadKeyed::CanReturnHole() const {
This name is kind of disambiguate, it took me a while to figure out that it's
used by the use-sites of this value. As discussed offline we should rename this
to something like "UsesMustHandleHole()".

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

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen-instructions...
src/hydrogen-instructions.h:5415: class HTrapAllocationMemento : public
HTemplateInstruction<2> {
Looks like this instruction only needs one operand. Can we adapt the template
parameter?

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen.cc#newcode641
src/hydrogen.cc:641: true_env_ = env->Copy();
I think it is safer not to preserve these environments in instance variables
(see comment about body_env_ below).

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen.cc#newcode688
src/hydrogen.cc:688: body_env_ = env->Copy();
Likewise.

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen.cc#newcode749
src/hydrogen.cc:749: body_env_->Push(increment_);
The body_env_ might be a stale environment in cases where you have nested loops
or an IfBuilder nested within a loop. I think it is safer to push the increment
to the current environment (i.e. HGraphBuilder::environment) and not preserve
the original body-environment in the LoopBuilder instance.

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen.cc#newcode988
src/hydrogen.cc:988: new(zone) HStoreNamedField(elements, map_field_name,
map_constant,
Strictly speaking this store needs to have the kChangesMaps side-effect. I think
in this case it is safe without that flag, but in general all HStoreNamedField
instructions that target the map-field should have that side-effect set. Maybe
this justifies a separate builder function for HStoreNamedField instructions
that target map-fields.

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen.cc#newcode1005
src/hydrogen.cc:1005: void HGraphBuilder::BuildCopyElements(HContext* context,
Beautifully simple, me gusta!

[danno, 2013-02-02 17:56:21]

Should be ready to go...

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2126: bool HLoadKeyed::CanReturnHole() const {
On 2013/02/01 13:15:08, Michael Starzinger wrote:
> This name is kind of disambiguate, it took me a while to figure out that it's
> used by the use-sites of this value. As discussed offline we should rename
this
> to something like "UsesMustHandleHole()".

Done.

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

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen-instructions...
src/hydrogen-instructions.h:5415: class HTrapAllocationMemento : public
HTemplateInstruction<2> {
On 2013/02/01 13:15:08, Michael Starzinger wrote:
> Looks like this instruction only needs one operand. Can we adapt the template
> parameter?

Done.

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen.cc#newcode641
src/hydrogen.cc:641: true_env_ = env->Copy();
On 2013/02/01 13:15:08, Michael Starzinger wrote:
> I think it is safer not to preserve these environments in instance variables
> (see comment about body_env_ below).

Done.

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen.cc#newcode688
src/hydrogen.cc:688: body_env_ = env->Copy();
On 2013/02/01 13:15:08, Michael Starzinger wrote:
> Likewise.

Done.

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen.cc#newcode749
src/hydrogen.cc:749: body_env_->Push(increment_);
On 2013/02/01 13:15:08, Michael Starzinger wrote:
> The body_env_ might be a stale environment in cases where you have nested
loops
> or an IfBuilder nested within a loop. I think it is safer to push the
increment
> to the current environment (i.e. HGraphBuilder::environment) and not preserve
> the original body-environment in the LoopBuilder instance.

Done.

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen.cc#newcode988
src/hydrogen.cc:988: new(zone) HStoreNamedField(elements, map_field_name,
map_constant,
On 2013/02/01 13:15:08, Michael Starzinger wrote:
> Strictly speaking this store needs to have the kChangesMaps side-effect. I
think
> in this case it is safe without that flag, but in general all HStoreNamedField
> instructions that target the map-field should have that side-effect set. Maybe
> this justifies a separate builder function for HStoreNamedField instructions
> that target map-fields.

Done.

https://codereview.chromium.org/11659022/diff/50002/src/hydrogen.cc#newcode1005
src/hydrogen.cc:1005: void HGraphBuilder::BuildCopyElements(HContext* context,
On 2013/02/01 13:15:08, Michael Starzinger wrote:
> Beautifully simple, me gusta!

Done.

[Michael Starzinger, 2013-02-04 10:15:24]

LGTM with two last comments about the test.

https://codereview.chromium.org/11659022/diff/37080/test/mjsunit/mjsunit.status
File test/mjsunit/mjsunit.status (right):

https://codereview.chromium.org/11659022/diff/37080/test/mjsunit/mjsunit.stat...
test/mjsunit/mjsunit.status:157: generated-transition-stub: PASS, SKIP if $mode
== debug
This is in the MIPS-only section, better move it up into the general sections.

https://codereview.chromium.org/11659022/diff/37080/test/mjsunit/regress/gene...
File test/mjsunit/regress/generated-transition-stub.js (right):

https://codereview.chromium.org/11659022/diff/37080/test/mjsunit/regress/gene...
test/mjsunit/regress/generated-transition-stub.js:1: // Copyright 2013 the V8
project authors. All rights reserved.
Not really a regression test, let's move it to either test/mjsunit or
test/mjsunit/compiler.