From c9591f005e6f76a75f86f310e188b56253e6a647 Mon Sep 17 00:00:00 2001 From: "mvstanton@chromium.org" Date: Mon, 26 Aug 2013 12:28:08 +0000 Subject: [PATCH] Store mode for keyed stores should be passed in from type feedback regardless of the map used in polymorphic stores. BUG= R=jkummerow@chromium.org, verwaest@chromium.org Review URL: https://codereview.chromium.org/21058003 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@16323 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/code-stubs-hydrogen.cc | 14 ++--- src/hydrogen.cc | 117 +++++++++++++++-------------------- src/hydrogen.h | 5 +- test/mjsunit/array-store-and-grow.js | 47 ++++++++++++++ 4 files changed, 107 insertions(+), 76 deletions(-) diff --git a/src/code-stubs-hydrogen.cc b/src/code-stubs-hydrogen.cc index 9bbab9a..3e18138 100644 --- a/src/code-stubs-hydrogen.cc +++ b/src/code-stubs-hydrogen.cc @@ -495,7 +495,7 @@ Handle CreateAllocationSiteStub::GenerateCode() { template <> HValue* CodeStubGraphBuilder::BuildCodeStub() { HInstruction* load = BuildUncheckedMonomorphicElementAccess( - GetParameter(0), GetParameter(1), NULL, NULL, + GetParameter(0), GetParameter(1), NULL, casted_stub()->is_js_array(), casted_stub()->elements_kind(), false, NEVER_RETURN_HOLE, STANDARD_STORE); return load; @@ -540,7 +540,7 @@ Handle KeyedLoadFieldStub::GenerateCode() { template <> HValue* CodeStubGraphBuilder::BuildCodeStub() { BuildUncheckedMonomorphicElementAccess( - GetParameter(0), GetParameter(1), GetParameter(2), NULL, + GetParameter(0), GetParameter(1), GetParameter(2), casted_stub()->is_js_array(), casted_stub()->elements_kind(), true, NEVER_RETURN_HOLE, casted_stub()->store_mode()); @@ -888,11 +888,11 @@ HValue* CodeStubGraphBuilder::BuildCodeStub() { casted_stub()->to_kind(), casted_stub()->is_jsarray()); - BuildUncheckedMonomorphicElementAccess(object, key, value, NULL, - casted_stub()->is_jsarray(), - casted_stub()->to_kind(), - true, ALLOW_RETURN_HOLE, - casted_stub()->store_mode()); + BuildUncheckedMonomorphicElementAccess(object, key, value, + casted_stub()->is_jsarray(), + casted_stub()->to_kind(), + true, ALLOW_RETURN_HOLE, + casted_stub()->store_mode()); } return value; diff --git a/src/hydrogen.cc b/src/hydrogen.cc index fd4cc35..95090d5 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -1219,10 +1219,9 @@ void HGraphBuilder::BuildTransitionElementsKind(HValue* object, HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess( - HValue* object, + HValue* checked_object, HValue* key, HValue* val, - HCheckMaps* checked_object, bool is_js_array, ElementsKind elements_kind, bool is_store, @@ -1237,14 +1236,12 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess( // generated store code. if ((elements_kind == FAST_HOLEY_ELEMENTS) || (elements_kind == FAST_ELEMENTS && is_store)) { - if (checked_object != NULL) { - checked_object->ClearGVNFlag(kDependsOnElementsKind); - } + checked_object->ClearGVNFlag(kDependsOnElementsKind); } - if (checked_object != NULL) object = checked_object; + bool fast_smi_only_elements = IsFastSmiElementsKind(elements_kind); bool fast_elements = IsFastObjectElementsKind(elements_kind); - HValue* elements = AddLoadElements(object); + HValue* elements = AddLoadElements(checked_object); if (is_store && (fast_elements || fast_smi_only_elements) && store_mode != STORE_NO_TRANSITION_HANDLE_COW) { HCheckMaps* check_cow_map = Add( @@ -1254,7 +1251,7 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess( HInstruction* length = NULL; if (is_js_array) { length = Add( - object, HObjectAccess::ForArrayLength(elements_kind)); + checked_object, HObjectAccess::ForArrayLength(elements_kind)); } else { length = AddLoadFixedArrayLength(elements); } @@ -1301,8 +1298,9 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess( if (IsGrowStoreMode(store_mode)) { NoObservableSideEffectsScope no_effects(this); - elements = BuildCheckForCapacityGrow(object, elements, elements_kind, - length, key, is_js_array); + elements = BuildCheckForCapacityGrow(checked_object, elements, + elements_kind, length, key, + is_js_array); checked_key = key; } else { checked_key = Add(key, length); @@ -1310,9 +1308,8 @@ HInstruction* HGraphBuilder::BuildUncheckedMonomorphicElementAccess( if (is_store && (fast_elements || fast_smi_only_elements)) { if (store_mode == STORE_NO_TRANSITION_HANDLE_COW) { NoObservableSideEffectsScope no_effects(this); - - elements = BuildCopyElementsOnWrite(object, elements, elements_kind, - length); + elements = BuildCopyElementsOnWrite(checked_object, elements, + elements_kind, length); } else { HCheckMaps* check_cow_map = Add( elements, isolate()->factory()->fixed_array_map(), @@ -5501,19 +5498,7 @@ HInstruction* HOptimizedGraphBuilder::BuildLoadKeyedGeneric(HValue* object, } -HInstruction* HOptimizedGraphBuilder::BuildMonomorphicElementAccess( - HValue* object, - HValue* key, - HValue* val, - HValue* dependency, - Handle map, - bool is_store, - KeyedAccessStoreMode store_mode) { - HCheckMaps* mapcheck = Add(object, map, top_info(), dependency); - if (dependency) { - mapcheck->ClearGVNFlag(kDependsOnElementsKind); - } - +LoadKeyedHoleMode HOptimizedGraphBuilder::BuildKeyedHoleMode(Handle map) { // Loads from a "stock" fast holey double arrays can elide the hole check. LoadKeyedHoleMode load_mode = NEVER_RETURN_HOLE; if (*map == isolate()->get_initial_js_array_map(FAST_HOLEY_DOUBLE_ELEMENTS) && @@ -5525,10 +5510,30 @@ HInstruction* HOptimizedGraphBuilder::BuildMonomorphicElementAccess( graph()->MarkDependsOnEmptyArrayProtoElements(); } + return load_mode; +} + + +HInstruction* HOptimizedGraphBuilder::BuildMonomorphicElementAccess( + HValue* object, + HValue* key, + HValue* val, + HValue* dependency, + Handle map, + bool is_store, + KeyedAccessStoreMode store_mode) { + HCheckMaps* checked_object = Add(object, map, top_info(), + dependency); + if (dependency) { + checked_object->ClearGVNFlag(kDependsOnElementsKind); + } + + LoadKeyedHoleMode load_mode = BuildKeyedHoleMode(map); return BuildUncheckedMonomorphicElementAccess( - object, key, val, - mapcheck, map->instance_type() == JS_ARRAY_TYPE, - map->elements_kind(), is_store, load_mode, store_mode); + checked_object, key, val, + map->instance_type() == JS_ARRAY_TYPE, + map->elements_kind(), is_store, + load_mode, store_mode); } @@ -5582,14 +5587,14 @@ HInstruction* HOptimizedGraphBuilder::TryBuildConsolidatedElementLoad( } if (!has_double_maps && !has_smi_or_object_maps) return NULL; - HCheckMaps* check_maps = Add(object, maps); + HCheckMaps* checked_object = Add(object, maps); // FAST_ELEMENTS is considered more general than FAST_HOLEY_SMI_ELEMENTS. // If we've seen both, the consolidated load must use FAST_HOLEY_ELEMENTS. ElementsKind consolidated_elements_kind = has_seen_holey_elements ? GetHoleyElementsKind(most_general_consolidated_map->elements_kind()) : most_general_consolidated_map->elements_kind(); HInstruction* instr = BuildUncheckedMonomorphicElementAccess( - object, key, val, check_maps, + checked_object, key, val, most_general_consolidated_map->instance_type() == JS_ARRAY_TYPE, consolidated_elements_kind, false, NEVER_RETURN_HOLE, STANDARD_STORE); @@ -5678,12 +5683,8 @@ HValue* HOptimizedGraphBuilder::HandlePolymorphicElementAccess( return is_store ? NULL : instr; } - HInstruction* checked_object = - AddInstruction(HCheckInstanceType::NewIsSpecObject(object, zone())); HBasicBlock* join = graph()->CreateBasicBlock(); - HInstruction* elements = AddLoadElements(checked_object); - for (int i = 0; i < untransitionable_maps.length(); ++i) { Handle map = untransitionable_maps[i]; ElementsKind elements_kind = map->elements_kind(); @@ -5694,40 +5695,22 @@ HValue* HOptimizedGraphBuilder::HandlePolymorphicElementAccess( current_block()->Finish(mapcompare); set_current_block(this_map); - HInstruction* checked_key = NULL; HInstruction* access = NULL; - if (IsFastElementsKind(elements_kind)) { - if (is_store && !IsFastDoubleElementsKind(elements_kind)) { - Add( - elements, isolate()->factory()->fixed_array_map(), - top_info(), mapcompare); - } - if (map->instance_type() == JS_ARRAY_TYPE) { - HInstruction* length = Add( - mapcompare, HObjectAccess::ForArrayLength(elements_kind)); - checked_key = Add(key, length); - } else { - HInstruction* length = AddLoadFixedArrayLength(elements); - checked_key = Add(key, length); - } - access = AddFastElementAccess( - elements, checked_key, val, mapcompare, - elements_kind, is_store, NEVER_RETURN_HOLE, STANDARD_STORE); - } else if (IsDictionaryElementsKind(elements_kind)) { - if (is_store) { - access = AddInstruction(BuildStoreKeyedGeneric(object, key, val)); - } else { - access = AddInstruction(BuildLoadKeyedGeneric(object, key)); - } + if (IsDictionaryElementsKind(elements_kind)) { + access = is_store + ? AddInstruction(BuildStoreKeyedGeneric(object, key, val)) + : AddInstruction(BuildLoadKeyedGeneric(object, key)); } else { - ASSERT(IsExternalArrayElementsKind(elements_kind)); - HInstruction* length = AddLoadFixedArrayLength(elements); - checked_key = Add(key, length); - HLoadExternalArrayPointer* external_elements = - Add(elements); - access = AddExternalArrayElementAccess( - external_elements, checked_key, val, - mapcompare, elements_kind, is_store); + ASSERT(IsFastElementsKind(elements_kind) || + IsExternalArrayElementsKind(elements_kind)); + LoadKeyedHoleMode load_mode = BuildKeyedHoleMode(map); + // Happily, mapcompare is a checked object. + access = BuildUncheckedMonomorphicElementAccess( + mapcompare, key, val, + map->instance_type() == JS_ARRAY_TYPE, + elements_kind, is_store, + load_mode, + store_mode); } *has_side_effects |= access->HasObservableSideEffects(); // The caller will use has_side_effects and add a correct Simulate. diff --git a/src/hydrogen.h b/src/hydrogen.h index 6128942..2b9fd96 100644 --- a/src/hydrogen.h +++ b/src/hydrogen.h @@ -1225,10 +1225,9 @@ class HGraphBuilder { bool is_jsarray); HInstruction* BuildUncheckedMonomorphicElementAccess( - HValue* object, + HValue* checked_object, HValue* key, HValue* val, - HCheckMaps* mapcheck, bool is_js_array, ElementsKind elements_kind, bool is_store, @@ -1982,6 +1981,8 @@ class HOptimizedGraphBuilder V8_FINAL HValue* val, SmallMapList* maps); + LoadKeyedHoleMode BuildKeyedHoleMode(Handle map); + HInstruction* BuildMonomorphicElementAccess(HValue* object, HValue* key, HValue* val, diff --git a/test/mjsunit/array-store-and-grow.js b/test/mjsunit/array-store-and-grow.js index a03a753..5ac95e9 100644 --- a/test/mjsunit/array-store-and-grow.js +++ b/test/mjsunit/array-store-and-grow.js @@ -187,6 +187,7 @@ array_store_1(a, 0, 0.5); assertEquals(0.5, a[0]); assertEquals(0.5, array_store_1([], 0, 0.5)); + // Verify that a grow store will deoptimize if the max gap (difference between // the end of an array capacity and a new index) is passed. The wrapper is to // make sure array_store_10 isn't inlined. @@ -207,3 +208,49 @@ assertEquals(0.5, array_store_1([], 0, 0.5)); %ClearFunctionTypeFeedback(grow_store); })(); + +// Verify that a polymorphic store and grow IC when crankshafted is still +// a grow IC (earlier it would revert to a standard store in the polymorphic +// case). +(function() { + function f(o, k, v) { + o[k] = v; + } + + a = [3.5]; + f(a, 1, "hi"); // DOUBLE packed array -> tagged packed grow + a = {}; + a.p = "property"; + a[0] = 1; + f(a, 0, 5.4); + + %OptimizeFunctionOnNextCall(f); + // Should be a polymorphic grow stub. If not a grow stub it will deopt. + f(new Array("hi"), 1, 3); + assertOptimized(f); + %ClearFunctionTypeFeedback(f); +})(); + + +// Now verify that a polymorphic store (non-growing) IC when crankshafted WILL +// deopt if you pass an element out of bounds. +(function() { + function f(o, k, v) { + o[k] = v; + } + + a = [3.5]; + f(a, 0, "hi"); // DOUBLE packed array -> tagged packed grow + a = {}; + a.p = "property"; + a[0] = 1; + f(a, 0, 5.4); + + %OptimizeFunctionOnNextCall(f); + f(new Array("hi"), 0, 3); + assertOptimized(f); + // An attempt to grow should cause deopt + f(new Array("hi"), 1, 3); + assertUnoptimized(f); + %ClearFunctionTypeFeedback(f); +})(); -- 2.7.4