From 3143f5f88b32607d12140fbf34b3ff74dde610a7 Mon Sep 17 00:00:00 2001 From: "verwaest@chromium.org" Date: Wed, 31 Jul 2013 10:08:05 +0000 Subject: [PATCH] Remove special handling of fields in combination with elements transitions in Crankshaft. Instead first try to keep ICs monomorphic if elements kinds generalize. If that fails, use standard polymorphic handling. The Try*PolymorphicAsMonomorphic methods will automatically produce code similar to the previous approach using CheckMapsWithTransitions. BUG= R=hpayer@chromium.org Review URL: https://chromiumcodereview.appspot.com/21107004 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@15982 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/hydrogen-instructions.cc | 33 ------------------ src/hydrogen-instructions.h | 3 -- src/hydrogen.cc | 73 +++------------------------------------- src/hydrogen.h | 3 -- src/ia32/lithium-codegen-ia32.cc | 3 -- src/ic.cc | 14 +++++--- src/x64/lithium-codegen-x64.cc | 3 -- 7 files changed, 15 insertions(+), 117 deletions(-) diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc index 87dad6f..01b6e70 100644 --- a/src/hydrogen-instructions.cc +++ b/src/hydrogen-instructions.cc @@ -3298,39 +3298,6 @@ HCheckMaps* HCheckMaps::New(HValue* value, } -HCheckMaps* HCheckMaps::NewWithTransitions(HValue* value, - Handle map, - Zone* zone, - CompilationInfo* info) { - HCheckMaps* check_map = new(zone) HCheckMaps(value, zone, value); - check_map->map_set_.Add(map, zone); - - // Since transitioned elements maps of the initial map don't fail the map - // check, the CheckMaps instruction doesn't need to depend on ElementsKinds. - check_map->ClearGVNFlag(kDependsOnElementsKind); - - ElementsKind kind = map->elements_kind(); - bool packed = IsFastPackedElementsKind(kind); - while (CanTransitionToMoreGeneralFastElementsKind(kind, packed)) { - kind = GetNextMoreGeneralFastElementsKind(kind, packed); - Map* transitioned_map = - map->LookupElementsTransitionMap(kind); - if (transitioned_map) { - check_map->map_set_.Add(Handle(transitioned_map), zone); - } - }; - - if (map->CanOmitMapChecks() && - value->IsConstant() && - HConstant::cast(value)->InstanceOf(map)) { - check_map->omit(info); - } - - check_map->map_set_.Sort(); - return check_map; -} - - void HCheckMaps::FinalizeUniqueValueId() { if (!map_unique_ids_.is_empty()) return; Zone* zone = block()->zone(); diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h index 08667c9..1b9af53 100644 --- a/src/hydrogen-instructions.h +++ b/src/hydrogen-instructions.h @@ -2726,9 +2726,6 @@ class HCheckMaps: public HTemplateInstruction<2> { return check_map; } - static HCheckMaps* NewWithTransitions(HValue* value, Handle map, - Zone* zone, CompilationInfo* info); - bool CanOmitMapChecks() { return omit_; } virtual bool HasEscapingOperandAt(int index) { return false; } diff --git a/src/hydrogen.cc b/src/hydrogen.cc index f1eaa22..0e103ca 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -4503,14 +4503,6 @@ void HOptimizedGraphBuilder::AddCheckMap(HValue* object, Handle map) { } -void HOptimizedGraphBuilder::AddCheckMapsWithTransitions(HValue* object, - Handle map) { - BuildCheckHeapObject(object); - AddInstruction(HCheckMaps::NewWithTransitions( - object, map, zone(), top_info())); -} - - HInstruction* HOptimizedGraphBuilder::BuildStoreNamedField( HValue* object, Handle name, @@ -4610,7 +4602,7 @@ HInstruction* HOptimizedGraphBuilder::BuildStoreNamedMonomorphic( // Handle a store to a known field. LookupResult lookup(isolate()); if (ComputeLoadStoreField(map, name, &lookup, true)) { - AddCheckMapsWithTransitions(object, map); + AddCheckMap(object, map); return BuildStoreNamedField(object, name, value, map, &lookup); } @@ -5390,7 +5382,7 @@ HInstruction* HOptimizedGraphBuilder::BuildLoadNamedMonomorphic( // Handle access to various length properties if (name->Equals(isolate()->heap()->length_string())) { if (map->instance_type() == JS_ARRAY_TYPE) { - AddCheckMapsWithTransitions(object, map); + AddCheckMap(object, map); return new(zone()) HLoadNamedField(object, HObjectAccess::ForArrayLength(map->elements_kind())); } @@ -5932,7 +5924,7 @@ void HOptimizedGraphBuilder::AddCheckConstantFunction( // Constant functions have the nice property that the map will change if they // are overwritten. Therefore it is enough to check the map of the holder and // its prototypes. - AddCheckMapsWithTransitions(receiver, receiver_map); + AddCheckMap(receiver, receiver_map); AddCheckPrototypeMaps(holder, receiver_map); } @@ -6885,55 +6877,6 @@ bool HOptimizedGraphBuilder::TryCallApply(Call* expr) { } -// Checks if all maps in |types| are from the same family, i.e., are elements -// transitions of each other. Returns either NULL if they are not from the same -// family, or a Map* indicating the map with the first elements kind of the -// family that is in the list. -static Map* CheckSameElementsFamily(SmallMapList* types) { - if (types->length() <= 1) return NULL; - // Check if all maps belong to the same transition family. - Map* kinds[kFastElementsKindCount]; - Map* first_map = *types->first(); - ElementsKind first_kind = first_map->elements_kind(); - if (!IsFastElementsKind(first_kind)) return NULL; - int first_index = GetSequenceIndexFromFastElementsKind(first_kind); - int last_index = first_index; - - for (int i = 0; i < kFastElementsKindCount; i++) kinds[i] = NULL; - - kinds[first_index] = first_map; - - for (int i = 1; i < types->length(); ++i) { - Map* map = *types->at(i); - ElementsKind elements_kind = map->elements_kind(); - if (!IsFastElementsKind(elements_kind)) return NULL; - int index = GetSequenceIndexFromFastElementsKind(elements_kind); - if (index < first_index) { - first_index = index; - } else if (index > last_index) { - last_index = index; - } else if (kinds[index] != map) { - return NULL; - } - kinds[index] = map; - } - - Map* current = kinds[first_index]; - for (int i = first_index + 1; i <= last_index; i++) { - Map* next = kinds[i]; - if (next != NULL) { - ElementsKind current_kind = next->elements_kind(); - if (next != current->LookupElementsTransitionMap(current_kind)) { - return NULL; - } - current = next; - } - } - - return kinds[first_index]; -} - - void HOptimizedGraphBuilder::VisitCall(Call* expr) { ASSERT(!HasStackOverflow()); ASSERT(current_block() != NULL); @@ -6979,12 +6922,6 @@ void HOptimizedGraphBuilder::VisitCall(Call* expr) { receiver_map = (types == NULL || types->is_empty()) ? Handle::null() : types->first(); - } else { - Map* family_map = CheckSameElementsFamily(types); - if (family_map != NULL) { - receiver_map = Handle(family_map); - monomorphic = expr->ComputeTarget(receiver_map, name); - } } HValue* receiver = @@ -8172,8 +8109,8 @@ void HOptimizedGraphBuilder::VisitCompareOperation(CompareOperation* expr) { // Can we get away with map check and not instance type check? if (combined_type->IsClass()) { Handle map = combined_type->AsClass(); - AddCheckMapsWithTransitions(left, map); - AddCheckMapsWithTransitions(right, map); + AddCheckMap(left, map); + AddCheckMap(right, map); HCompareObjectEqAndBranch* result = new(zone()) HCompareObjectEqAndBranch(left, right); result->set_position(expr->position()); diff --git a/src/hydrogen.h b/src/hydrogen.h index 8484cd1..99428c1 100644 --- a/src/hydrogen.h +++ b/src/hydrogen.h @@ -1892,9 +1892,6 @@ class HOptimizedGraphBuilder: public HGraphBuilder, public AstVisitor { void AddCheckMap(HValue* object, Handle map); - void AddCheckMapsWithTransitions(HValue* object, - Handle map); - void BuildStoreNamed(Expression* expression, BailoutId id, int position, diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc index ef3761e..ed9d3ff 100644 --- a/src/ia32/lithium-codegen-ia32.cc +++ b/src/ia32/lithium-codegen-ia32.cc @@ -3165,9 +3165,6 @@ static bool CompactEmit(SmallMapList* list, int i, Isolate* isolate) { Handle map = list->at(i); - // If the map has ElementsKind transitions, we will generate map checks - // for each kind in __ CompareMap(..., ALLOW_ELEMENTS_TRANSITION_MAPS). - if (map->HasElementsTransition()) return false; LookupResult lookup(isolate); map->LookupDescriptor(NULL, *name, &lookup); return lookup.IsField() || lookup.IsConstant(); diff --git a/src/ic.cc b/src/ic.cc index 269754b..a55160a 100644 --- a/src/ic.cc +++ b/src/ic.cc @@ -233,16 +233,22 @@ static bool TryRemoveInvalidPrototypeDependentStub(Code* target, // The stub is not in the cache. We've ruled out all other kinds of failure // except for proptotype chain changes, a deprecated map, a map that's - // different from the one that the stub expects, or a constant global property - // that will become mutable. Threat all those situations as prototype failures - // (stay monomorphic if possible). + // different from the one that the stub expects, elements kind changes, or a + // constant global property that will become mutable. Threat all those + // situations as prototype failures (stay monomorphic if possible). // If the IC is shared between multiple receivers (slow dictionary mode), then // the map cannot be deprecated and the stub invalidated. if (cache_holder == OWN_MAP) { Map* old_map = target->FindFirstMap(); if (old_map == map) return true; - if (old_map != NULL && old_map->is_deprecated()) return true; + if (old_map != NULL) { + if (old_map->is_deprecated()) return true; + if (IsMoreGeneralElementsKindTransition(old_map->elements_kind(), + map->elements_kind())) { + return true; + } + } } if (receiver->IsGlobalObject()) { diff --git a/src/x64/lithium-codegen-x64.cc b/src/x64/lithium-codegen-x64.cc index 0f72e29..0bd9b16 100644 --- a/src/x64/lithium-codegen-x64.cc +++ b/src/x64/lithium-codegen-x64.cc @@ -2780,9 +2780,6 @@ static bool CompactEmit(SmallMapList* list, int i, Isolate* isolate) { Handle map = list->at(i); - // If the map has ElementsKind transitions, we will generate map checks - // for each kind in __ CompareMap(..., ALLOW_ELEMENTS_TRANSITION_MAPS). - if (map->HasElementsTransition()) return false; LookupResult lookup(isolate); map->LookupDescriptor(NULL, *name, &lookup); return lookup.IsField() || lookup.IsConstant(); -- 2.7.4