Implement many KeyedStoreStubs using Crankshaft

Implement many KeyedStoreStubs using Crankshaft

- Addition of a compiled hydrogen stub for KeyedStores.
- Inlining of "grow" stubs into OPTIMIZED_FUNCTIONs
- Addition of new "ignore OOB" ic stub that silently swallows out-of-bounds stores to external typed arrays.
- Addition of new "copy-on-write" ic stub that inlines allocation and copying operations for cow array 
- New stub are generated with Crankshaft, so they are automatically inlined into OPTIMIZED_FUNCTIONs

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

[danno, 2013-03-07 11:19:49]

PTAL

[Jakob Kummerow, 2013-03-11 16:36:07]

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

https://codereview.chromium.org/12221064/diff/49001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:281: Isolate* isolate = graph()->isolate();
not necessary, can use [this->]isolate() directly

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode626
src/hydrogen.cc:626: return GetConstantInt32(&constant_2_, 1);
1 is the new 2?

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1012
src/hydrogen.cc:1012: IfBuilder length_checker_if(this, ast_id);
nit: Drop the _if. Just length_checker. It's cleaner.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1019
src/hydrogen.cc:1019: IfBuilder capacity_checker_if(this, ast_id);
again

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1045
src/hydrogen.cc:1045: Isolate* isolate = graph()->isolate();
no need for this, HGraphBuilder has an isolate() accessor.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1073
src/hydrogen.cc:1073: Heap* heap = info_->isolate()->heap();
s/info_->// (or s/info_/this/ if you like needless verbosity)

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1075
src/hydrogen.cc:1075: AddInstruction(new(zone) HLoadNamedField(elements, true,
Are you intentionally not using an HCompareMap instruction here?

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1157
src/hydrogen.cc:1157: IfBuilder oob_ignore_check(this, previous_id);
How about we call this IfBuilder length_checker (analogously to
BuildCheckForCapacityGrow above)...

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1159
src/hydrogen.cc:1159: IfBuilder oob_ignore_check2(this, previous_id);
...and this one negative_checker?

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1166
src/hydrogen.cc:1166: AddSoftDeoptimize();
You don't want a soft deopt here. HSoftDeoptimize is intended for blocks where
we don't have type feedback yet, not for things we generally can't handle in
optimized code; if FLAG_always_opt, AddSoftDeoptimize() does nothing.

You should use an HDeoptimize here instead, which you can insert via
current_block()->FinishExitWithDeoptimization. Too bad that doesn't fit into the
IfBuilder infrastructure.

Alternative: (ab)use an HBoundsCheck. It does an unsigned comparison, so if I'm
not mistaken you can get the desired behavior (deopt if checked_index < 0) with
a bounds check against 0x10000000 (aka kMinInt).

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1193
src/hydrogen.cc:1193: val->type().IsTagged() && !val->type().IsSmi()) {
Fun fact: there is no HType (except "uninitialized") for which type().IsTagged()
would return "false".

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1209
src/hydrogen.cc:1209: length);
nit: fits on the previous line

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1212
src/hydrogen.cc:1212: elements,
Isolate::Current()->factory()->fixed_array_map(), zone);
s/Isolate::Current()/isolate()/

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1305
src/hydrogen.cc:1305: AddInstruction(HDiv::New(zone,
prefer HSar (then you won't need GetConstant2() at all).

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1318
src/hydrogen.cc:1318: AddInstruction(new(zone) HConstant(4,
Representation::Integer32()));
The C++ version (NewElementsCapacity in objects.h) adds 16 here.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1331
src/hydrogen.cc:1331: Heap* heap = info_->isolate()->heap();
s/info_->//

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1333
src/hydrogen.cc:1333: ? kDoubleSize
nit: fits on previous line. For your copy/paste convenience:

  int element_size = IsFastDoubleElementsKind(kind) ? kDoubleSize
                                                    : kPointerSize;

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1336
src/hydrogen.cc:1336: max_size -= sizeof(JSArray) / element_size;
I think you mean s/sizeof(JSArray)/JSArray::kSize/.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1340
src/hydrogen.cc:1340: // Since we're forcing Integer32 representation for this
HBoundsCheck,
I'd prefer to make this explicit by passing (_, _, DONT_ALLOW_SMI_KEY,
Representation::Integer32()) to the ctor.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1364
src/hydrogen.cc:1364: Isolate* isolate = graph()->isolate();
no need for this, just use [this->]isolate() directly.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1385
src/hydrogen.cc:1385: Factory* factory = info_->isolate()->factory();
s/info_->//

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1400
src/hydrogen.cc:1400: elements_kind));
nit: fits on previous line

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode7397
src/hydrogen.cc:7397: is_store, expr->GetStoreMode());
nit: fits on previous line

https://codereview.chromium.org/12221064/diff/49001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (left):

https://codereview.chromium.org/12221064/diff/49001/src/ia32/stub-cache-ia32....
src/ia32/stub-cache-ia32.cc:3338: void
KeyedStoreStubCompiler::GenerateStoreExternalArray(
Love this change! :-)

https://codereview.chromium.org/12221064/diff/49001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/12221064/diff/49001/src/ic.cc#newcode1817
src/ic.cc:1817: key->ToSmi()->To<Smi>(&smi_key);
nit: drop the "<Smi>", just "...->To(&smi_key)".

https://codereview.chromium.org/12221064/diff/49001/src/ic.h
File src/ic.h (right):

https://codereview.chromium.org/12221064/diff/49001/src/ic.h#newcode498
src/ic.h:498: explicit StoreIC(FrameDepth depth, Isolate* isolate)
nit: no "explicit" necessary

[danno, 2013-03-13 15:36:26]

PTAL. Patch set 17 is feedback delta, patch set 18 is the result merged with
ToT.

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

https://codereview.chromium.org/12221064/diff/49001/src/code-stubs-hydrogen.c...
src/code-stubs-hydrogen.cc:281: Isolate* isolate = graph()->isolate();
On 2013/03/11 16:36:07, Jakob wrote:
> not necessary, can use [this->]isolate() directly

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode626
src/hydrogen.cc:626: return GetConstantInt32(&constant_2_, 1);
On 2013/03/11 16:36:07, Jakob wrote:
> 1 is the new 2?

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1012
src/hydrogen.cc:1012: IfBuilder length_checker_if(this, ast_id);
On 2013/03/11 16:36:07, Jakob wrote:
> nit: Drop the _if. Just length_checker. It's cleaner.

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1019
src/hydrogen.cc:1019: IfBuilder capacity_checker_if(this, ast_id);
On 2013/03/11 16:36:07, Jakob wrote:
> again

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1045
src/hydrogen.cc:1045: Isolate* isolate = graph()->isolate();
On 2013/03/11 16:36:07, Jakob wrote:
> no need for this, HGraphBuilder has an isolate() accessor.

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1073
src/hydrogen.cc:1073: Heap* heap = info_->isolate()->heap();
On 2013/03/11 16:36:07, Jakob wrote:
> s/info_->// (or s/info_/this/ if you like needless verbosity)

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1075
src/hydrogen.cc:1075: AddInstruction(new(zone) HLoadNamedField(elements, true,
On 2013/03/11 16:36:07, Jakob wrote:
> Are you intentionally not using an HCompareMap instruction here?

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1157
src/hydrogen.cc:1157: IfBuilder oob_ignore_check(this, previous_id);
On 2013/03/11 16:36:07, Jakob wrote:
> How about we call this IfBuilder length_checker (analogously to
> BuildCheckForCapacityGrow above)...

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1159
src/hydrogen.cc:1159: IfBuilder oob_ignore_check2(this, previous_id);
On 2013/03/11 16:36:07, Jakob wrote:
> ...and this one negative_checker?

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1166
src/hydrogen.cc:1166: AddSoftDeoptimize();
On 2013/03/11 16:36:07, Jakob wrote:
> You don't want a soft deopt here. HSoftDeoptimize is intended for blocks where
> we don't have type feedback yet, not for things we generally can't handle in
> optimized code; if FLAG_always_opt, AddSoftDeoptimize() does nothing.
> 
> You should use an HDeoptimize here instead, which you can insert via
> current_block()->FinishExitWithDeoptimization. Too bad that doesn't fit into
the
> IfBuilder infrastructure.
> 
> Alternative: (ab)use an HBoundsCheck. It does an unsigned comparison, so if
I'm
> not mistaken you can get the desired behavior (deopt if checked_index < 0)
with
> a bounds check against 0x10000000 (aka kMinInt).

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1193
src/hydrogen.cc:1193: val->type().IsTagged() && !val->type().IsSmi()) {
On 2013/03/11 16:36:07, Jakob wrote:
> Fun fact: there is no HType (except "uninitialized") for which
type().IsTagged()
> would return "false".

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1209
src/hydrogen.cc:1209: length);
On 2013/03/11 16:36:07, Jakob wrote:
> nit: fits on the previous line

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1212
src/hydrogen.cc:1212: elements,
Isolate::Current()->factory()->fixed_array_map(), zone);
On 2013/03/11 16:36:07, Jakob wrote:
> s/Isolate::Current()/isolate()/

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1305
src/hydrogen.cc:1305: AddInstruction(HDiv::New(zone,
On 2013/03/11 16:36:07, Jakob wrote:
> prefer HSar (then you won't need GetConstant2() at all).

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1318
src/hydrogen.cc:1318: AddInstruction(new(zone) HConstant(4,
Representation::Integer32()));
On 2013/03/11 16:36:07, Jakob wrote:
> The C++ version (NewElementsCapacity in objects.h) adds 16 here.

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1331
src/hydrogen.cc:1331: Heap* heap = info_->isolate()->heap();
On 2013/03/11 16:36:07, Jakob wrote:
> s/info_->//

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1333
src/hydrogen.cc:1333: ? kDoubleSize
On 2013/03/11 16:36:07, Jakob wrote:
> nit: fits on previous line. For your copy/paste convenience:
> 
>   int element_size = IsFastDoubleElementsKind(kind) ? kDoubleSize
>                                                     : kPointerSize;

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1336
src/hydrogen.cc:1336: max_size -= sizeof(JSArray) / element_size;
On 2013/03/11 16:36:07, Jakob wrote:
> I think you mean s/sizeof(JSArray)/JSArray::kSize/.

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1340
src/hydrogen.cc:1340: // Since we're forcing Integer32 representation for this
HBoundsCheck,
On 2013/03/11 16:36:07, Jakob wrote:
> I'd prefer to make this explicit by passing (_, _, DONT_ALLOW_SMI_KEY,
> Representation::Integer32()) to the ctor.

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1364
src/hydrogen.cc:1364: Isolate* isolate = graph()->isolate();
On 2013/03/11 16:36:07, Jakob wrote:
> no need for this, just use [this->]isolate() directly.

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1385
src/hydrogen.cc:1385: Factory* factory = info_->isolate()->factory();
On 2013/03/11 16:36:07, Jakob wrote:
> s/info_->//

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode1400
src/hydrogen.cc:1400: elements_kind));
On 2013/03/11 16:36:07, Jakob wrote:
> nit: fits on previous line

Done.

https://codereview.chromium.org/12221064/diff/49001/src/hydrogen.cc#newcode7397
src/hydrogen.cc:7397: is_store, expr->GetStoreMode());
On 2013/03/11 16:36:07, Jakob wrote:
> nit: fits on previous line

Done.

https://codereview.chromium.org/12221064/diff/49001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/12221064/diff/49001/src/ic.cc#newcode1817
src/ic.cc:1817: key->ToSmi()->To<Smi>(&smi_key);
On 2013/03/11 16:36:07, Jakob wrote:
> nit: drop the "<Smi>", just "...->To(&smi_key)".

Done.

https://codereview.chromium.org/12221064/diff/49001/src/ic.h
File src/ic.h (right):

https://codereview.chromium.org/12221064/diff/49001/src/ic.h#newcode498
src/ic.h:498: explicit StoreIC(FrameDepth depth, Isolate* isolate)
On 2013/03/11 16:36:07, Jakob wrote:
> nit: no "explicit" necessary

Done.

[Jakob Kummerow, 2013-03-14 15:00:20]

LGTM with nits.

https://codereview.chromium.org/12221064/diff/68001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12221064/diff/68001/src/hydrogen.cc#newcode1199
src/hydrogen.cc:1199: val->type().IsTagged() && !val->type().IsSmi()) {
I meant that you can remove the val->type().IsTagged() check. It always returns
true.

https://codereview.chromium.org/12221064/diff/68001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/12221064/diff/68001/src/hydrogen.h#newcode975
src/hydrogen.h:975: HInstruction* BeginIfMapsEqual(HValue* value, Handle<Map>
map);
nit: "BeginIfMapEquals" would be a bit more accurate.

[danno, 2013-03-20 10:04:34]

Addressed feedback.

https://codereview.chromium.org/12221064/diff/68001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/12221064/diff/68001/src/hydrogen.cc#newcode1199
src/hydrogen.cc:1199: val->type().IsTagged() && !val->type().IsSmi()) {
On 2013/03/14 15:00:20, Jakob wrote:
> I meant that you can remove the val->type().IsTagged() check. It always
returns
> true.

Done.

https://codereview.chromium.org/12221064/diff/68001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/12221064/diff/68001/src/hydrogen.h#newcode975
src/hydrogen.h:975: HInstruction* BeginIfMapsEqual(HValue* value, Handle<Map>
map);
On 2013/03/14 15:00:20, Jakob wrote:
> nit: "BeginIfMapEquals" would be a bit more accurate.

Done.

[Hannes Payer (out of office), 2013-03-20 10:22:35]

LGTM with a few nits.

As discussed offline, the test case does not work right now and should be
removed.

https://codereview.chromium.org/12221064/diff/85002/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

https://codereview.chromium.org/12221064/diff/85002/src/arm/stub-cache-arm.cc...
src/arm/stub-cache-arm.cc:2995: __ DispatchMap(r2, r3, receiver_map, stub,
DO_SMI_CHECK);
Can you declare Handle<Code> stube before the if/else statement and move __
DispatchMap(r2, r3, receiver_map, stub, DO_SMI_CHECK); below the if/else
statement since DispatchMap is duplicated? This should also apply to
stub-cache-ia32.cc and to stub-cache-x64.cc

https://codereview.chromium.org/12221064/diff/85002/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/12221064/diff/85002/src/ic.cc#newcode1327
src/ic.cc:1327: 
Can you bring back the newline?

https://codereview.chromium.org/12221064/diff/85002/src/ic.cc#newcode1652
src/ic.cc:1652: if ((store_mode == STORE_NO_TRANSITION_HANDLE_COW ||
This code should be executed if FLAG_compiled_keyed_stores is false.

[danno, 2013-03-20 10:37:33]

Committed patchset #20 manually as r14001 (presubmit successful).

[danno, 2013-03-20 11:44:10]

Addressed feedback and landed.

https://codereview.chromium.org/12221064/diff/85002/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

https://codereview.chromium.org/12221064/diff/85002/src/arm/stub-cache-arm.cc...
src/arm/stub-cache-arm.cc:2995: __ DispatchMap(r2, r3, receiver_map, stub,
DO_SMI_CHECK);
On 2013/03/20 10:22:35, Hannes Payer wrote:
> Can you declare Handle<Code> stube before the if/else statement and move __
> DispatchMap(r2, r3, receiver_map, stub, DO_SMI_CHECK); below the if/else
> statement since DispatchMap is duplicated? This should also apply to
> stub-cache-ia32.cc and to stub-cache-x64.cc

Done.

https://codereview.chromium.org/12221064/diff/85002/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/12221064/diff/85002/src/ic.cc#newcode1327
src/ic.cc:1327: 
On 2013/03/20 10:22:35, Hannes Payer wrote:
> Can you bring back the newline?

Done.

https://codereview.chromium.org/12221064/diff/85002/src/ic.cc#newcode1652
src/ic.cc:1652: if ((store_mode == STORE_NO_TRANSITION_HANDLE_COW ||
On 2013/03/20 10:22:35, Hannes Payer wrote:
> This code should be executed if FLAG_compiled_keyed_stores is false.

Done.