From 2b7d33572ab1f2887571623d8380ba01d9a03e84 Mon Sep 17 00:00:00 2001 From: "verwaest@chromium.org" Date: Wed, 12 Feb 2014 15:07:41 +0000 Subject: [PATCH] Use stability to only conditionally flush information from the CheckMaps table. BUG= R=ishell@chromium.org Review URL: https://codereview.chromium.org/153823003 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@19330 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/code-stubs-hydrogen.cc | 8 +- src/compiler.cc | 2 +- src/hydrogen-check-elimination.cc | 101 +++++++++++------- src/hydrogen-instructions.cc | 2 +- src/hydrogen-instructions.h | 49 ++++++++- src/hydrogen.cc | 25 +++-- test/cctest/test-deoptimization.cc | 2 + .../regress/regress-map-invalidation-2.js | 2 + 8 files changed, 133 insertions(+), 58 deletions(-) diff --git a/src/code-stubs-hydrogen.cc b/src/code-stubs-hydrogen.cc index f9b4d55dfc..4c646fea08 100644 --- a/src/code-stubs-hydrogen.cc +++ b/src/code-stubs-hydrogen.cc @@ -295,7 +295,8 @@ HValue* CodeStubGraphBuilder::BuildCodeStub() { // Check if the parameter is already a SMI or heap number. IfBuilder if_number(this); if_number.If(value); - if_number.OrIf(value, isolate()->factory()->heap_number_map()); + if_number.OrIf(value, isolate()->factory()->heap_number_map(), + top_info()); if_number.Then(); // Return the number. @@ -358,7 +359,8 @@ HValue* CodeStubGraphBuilder::BuildCodeStub() { HValue* elements = AddLoadElements(boilerplate); IfBuilder if_fixed_cow(this); - if_fixed_cow.If(elements, factory->fixed_cow_array_map()); + if_fixed_cow.If(elements, factory->fixed_cow_array_map(), + top_info()); if_fixed_cow.Then(); push_value = BuildCloneShallowArray(boilerplate, allocation_site, @@ -369,7 +371,7 @@ HValue* CodeStubGraphBuilder::BuildCodeStub() { if_fixed_cow.Else(); IfBuilder if_fixed(this); - if_fixed.If(elements, factory->fixed_array_map()); + if_fixed.If(elements, factory->fixed_array_map(), top_info()); if_fixed.Then(); push_value = BuildCloneShallowArray(boilerplate, allocation_site, diff --git a/src/compiler.cc b/src/compiler.cc index b773880b8c..6ebbe41f9c 100644 --- a/src/compiler.cc +++ b/src/compiler.cc @@ -1185,7 +1185,7 @@ Handle Compiler::GetOptimizedCode(Handle function, if (FLAG_trace_opt) { PrintF("[failed to optimize "); function->PrintName(); - PrintF("]\n"); + PrintF(": %s]\n", GetBailoutReason(info->bailout_reason())); } if (isolate->has_pending_exception()) isolate->clear_pending_exception(); diff --git a/src/hydrogen-check-elimination.cc b/src/hydrogen-check-elimination.cc index 5fdb3e3f3c..d5715a61ec 100644 --- a/src/hydrogen-check-elimination.cc +++ b/src/hydrogen-check-elimination.cc @@ -50,6 +50,7 @@ struct HCheckTableEntry { HValue* object_; // The object being approximated. NULL => invalid entry. HInstruction* check_; // The last check instruction. MapSet maps_; // The set of known maps for the object. + bool is_stable_; }; @@ -103,9 +104,10 @@ class HCheckTable : public ZoneObject { } default: { // If the instruction changes maps uncontrollably, drop everything. - if (instr->CheckChangesFlag(kMaps) || - instr->CheckChangesFlag(kOsrEntries)) { - Kill(); + if (instr->CheckChangesFlag(kOsrEntries)) { + Reset(); + } else if (instr->CheckChangesFlag(kMaps)) { + KillUnstableEntries(); } } // Improvements possible: @@ -154,6 +156,7 @@ class HCheckTable : public ZoneObject { if (old_entry->check_ != NULL && old_entry->check_->block()->Dominates(succ)) { new_entry->check_ = old_entry->check_; + new_entry->is_stable_ = old_entry->is_stable_; } else { // Leave it NULL till we meet a new check instruction for this object // in the control flow. @@ -175,7 +178,8 @@ class HCheckTable : public ZoneObject { HCheckTableEntry* pred_entry = copy->Find(phi_operand); if (pred_entry != NULL) { // Create an entry for a phi in the table. - copy->Insert(phi, NULL, pred_entry->maps_->Copy(phase_->zone())); + copy->Insert(phi, NULL, pred_entry->maps_->Copy(phase_->zone()), + pred_entry->is_stable_); } } } @@ -193,12 +197,13 @@ class HCheckTable : public ZoneObject { if (is_true_branch) { // Learn on the true branch of if(CompareMap(x)). if (entry == NULL) { - copy->Insert(object, cmp, cmp->map()); + copy->Insert(object, cmp, cmp->map(), cmp->is_stable()); } else { MapSet list = new(phase_->zone()) UniqueSet(); list->Add(cmp->map(), phase_->zone()); entry->maps_ = list; entry->check_ = cmp; + entry->is_stable_ = cmp->is_stable(); } } else { // Learn on the false branch of if(CompareMap(x)). @@ -217,10 +222,10 @@ class HCheckTable : public ZoneObject { HCheckTableEntry* re = copy->Find(right); if (le == NULL) { if (re != NULL) { - copy->Insert(left, NULL, re->maps_->Copy(zone)); + copy->Insert(left, NULL, re->maps_->Copy(zone), re->is_stable_); } } else if (re == NULL) { - copy->Insert(right, NULL, le->maps_->Copy(zone)); + copy->Insert(right, NULL, le->maps_->Copy(zone), le->is_stable_); } else { MapSet intersect = le->maps_->Intersect(re->maps_, zone); le->maps_ = intersect; @@ -247,8 +252,7 @@ class HCheckTable : public ZoneObject { HBasicBlock* pred_block, Zone* zone) { if (that->size_ == 0) { // If the other state is empty, simply reset. - size_ = 0; - cursor_ = 0; + Reset(); } else { int pred_index = succ->PredecessorIndexOf(pred_block); bool compact = false; @@ -271,6 +275,8 @@ class HCheckTable : public ZoneObject { } else { this_entry->maps_ = this_entry->maps_->Union(that_entry->maps_, phase_->zone()); + this_entry->is_stable_ = + this_entry->is_stable_ && that_entry->is_stable_; if (this_entry->check_ != that_entry->check_) { this_entry->check_ = NULL; } @@ -351,7 +357,8 @@ class HCheckTable : public ZoneObject { } } else { // No entry; insert a new one. - Insert(object, instr, instr->map_set().Copy(phase_->zone())); + Insert(object, instr, instr->map_set().Copy(phase_->zone()), + instr->is_stable()); } } @@ -413,7 +420,8 @@ class HCheckTable : public ZoneObject { } } else { // No prior information. - Insert(object, instr, map); + // TODO(verwaest): Tag map constants with stability. + Insert(object, instr, map, false); } } @@ -430,12 +438,14 @@ class HCheckTable : public ZoneObject { if (instr->has_transition()) { // This store transitions the object to a new map. Kill(object); - Insert(object, NULL, MapConstant(instr->transition())); + Insert(object, NULL, MapConstant(instr->transition()), + instr->is_stable()); } else if (IsMapAccess(instr->access())) { // This is a store directly to the map field of the object. Kill(object); if (!instr->value()->IsConstant()) return; - Insert(object, NULL, MapConstant(instr->value())); + // TODO(verwaest): Tag with stability. + Insert(object, NULL, MapConstant(instr->value()), false); } else { // If the instruction changes maps, it should be handled above. CHECK(!instr->CheckChangesFlag(kMaps)); @@ -485,12 +495,26 @@ class HCheckTable : public ZoneObject { } } - // Kill everything in the table. - void Kill() { + // Reset the table. + void Reset() { size_ = 0; cursor_ = 0; } + // Kill everything in the table. + void KillUnstableEntries() { + bool compact = false; + for (int i = 0; i < size_; i++) { + HCheckTableEntry* entry = &entries_[i]; + ASSERT(entry->object_ != NULL); + if (!entry->is_stable_) { + entry->object_ = NULL; + compact = true; + } + } + if (compact) Compact(); + } + // Kill everything in the table that may alias {object}. void Kill(HValue* object) { bool compact = false; @@ -572,17 +596,24 @@ class HCheckTable : public ZoneObject { return entry == NULL ? NULL : entry->maps_; } - void Insert(HValue* object, HInstruction* check, Unique map) { + void Insert(HValue* object, + HInstruction* check, + Unique map, + bool is_stable) { MapSet list = new(phase_->zone()) UniqueSet(); list->Add(map, phase_->zone()); - Insert(object, check, list); + Insert(object, check, list, is_stable); } - void Insert(HValue* object, HInstruction* check, MapSet maps) { + void Insert(HValue* object, + HInstruction* check, + MapSet maps, + bool is_stable) { HCheckTableEntry* entry = &entries_[cursor_++]; entry->object_ = object; entry->check_ = check; entry->maps_ = maps; + entry->is_stable_ = is_stable; // If the table becomes full, wrap around and overwrite older entries. if (cursor_ == kMaxTrackedObjects) cursor_ = 0; if (size_ < kMaxTrackedObjects) size_++; @@ -612,8 +643,7 @@ class HCheckTable : public ZoneObject { class HCheckMapsEffects : public ZoneObject { public: explicit HCheckMapsEffects(Zone* zone) - : maps_stored_(false), - stores_(5, zone) { } + : stores_(5, zone) { } inline bool Disabled() { return false; // Effects are _not_ disabled. @@ -621,27 +651,22 @@ class HCheckMapsEffects : public ZoneObject { // Process a possibly side-effecting instruction. void Process(HInstruction* instr, Zone* zone) { - switch (instr->opcode()) { - case HValue::kStoreNamedField: { - stores_.Add(HStoreNamedField::cast(instr), zone); - break; - } - case HValue::kOsrEntry: { - // Kill everything. Loads must not be hoisted past the OSR entry. - maps_stored_ = true; - } - default: { - maps_stored_ |= (instr->CheckChangesFlag(kMaps) | - instr->CheckChangesFlag(kElementsKind)); - } + if (instr->IsStoreNamedField()) { + stores_.Add(HStoreNamedField::cast(instr), zone); + } else { + flags_.Add(instr->ChangesFlags()); } } // Apply these effects to the given check elimination table. void Apply(HCheckTable* table) { - if (maps_stored_) { + if (flags_.Contains(kOsrEntries)) { + table->Reset(); + return; + } + if (flags_.Contains(kMaps) || flags_.Contains(kElementsKind)) { // Uncontrollable map modifications; kill everything. - table->Kill(); + table->KillUnstableEntries(); return; } @@ -656,14 +681,14 @@ class HCheckMapsEffects : public ZoneObject { // Union these effects with the other effects. void Union(HCheckMapsEffects* that, Zone* zone) { - maps_stored_ |= that->maps_stored_; + flags_.Add(that->flags_); for (int i = 0; i < that->stores_.length(); i++) { stores_.Add(that->stores_[i], zone); } } private: - bool maps_stored_ : 1; + GVNFlagSet flags_; ZoneList stores_; }; @@ -680,7 +705,7 @@ void HCheckEliminationPhase::Run() { } else { // Perform only local analysis. for (int i = 0; i < graph()->blocks()->length(); i++) { - table->Kill(); + table->Reset(); engine.AnalyzeOneBlock(graph()->blocks()->at(i), table); } } diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc index 7a62bb7a42..00087e4041 100644 --- a/src/hydrogen-instructions.cc +++ b/src/hydrogen-instructions.cc @@ -3107,7 +3107,7 @@ HCheckMaps* HCheckMaps::New(Zone* zone, CompilationInfo* info, HValue* typecheck) { HCheckMaps* check_map = new(zone) HCheckMaps(value, zone, typecheck); - check_map->Add(map, zone); + check_map->Add(map, info, zone); if (map->CanOmitMapChecks() && value->IsConstant() && HConstant::cast(value)->HasMap(map)) { diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h index 63368ce6ea..20a721273e 100644 --- a/src/hydrogen-instructions.h +++ b/src/hydrogen-instructions.h @@ -1508,8 +1508,10 @@ class HBranch V8_FINAL : public HUnaryControlInstruction { class HCompareMap V8_FINAL : public HUnaryControlInstruction { public: - DECLARE_INSTRUCTION_FACTORY_P2(HCompareMap, HValue*, Handle); - DECLARE_INSTRUCTION_FACTORY_P4(HCompareMap, HValue*, Handle, + DECLARE_INSTRUCTION_FACTORY_P3(HCompareMap, HValue*, Handle, + CompilationInfo*); + DECLARE_INSTRUCTION_FACTORY_P5(HCompareMap, HValue*, Handle, + CompilationInfo*, HBasicBlock*, HBasicBlock*); virtual bool KnownSuccessorBlock(HBasicBlock** block) V8_OVERRIDE { @@ -1535,6 +1537,10 @@ class HCompareMap V8_FINAL : public HUnaryControlInstruction { return Representation::Tagged(); } + bool is_stable() const { + return is_stable_; + } + DECLARE_CONCRETE_INSTRUCTION(CompareMap) protected: @@ -1543,14 +1549,22 @@ class HCompareMap V8_FINAL : public HUnaryControlInstruction { private: HCompareMap(HValue* value, Handle map, + CompilationInfo* info, HBasicBlock* true_target = NULL, HBasicBlock* false_target = NULL) : HUnaryControlInstruction(value, true_target, false_target), known_successor_index_(kNoKnownSuccessorIndex), map_(Unique(map)) { ASSERT(!map.is_null()); + is_stable_ = map->is_stable(); + + if (FLAG_check_elimination && is_stable_) { + map->AddDependentCompilationInfo( + DependentCode::kPrototypeCheckGroup, info); + } } int known_successor_index_; + bool is_stable_; Unique map_; }; @@ -2633,10 +2647,11 @@ class HCheckMaps V8_FINAL : public HTemplateInstruction<2> { HValue* typecheck = NULL); static HCheckMaps* New(Zone* zone, HValue* context, HValue* value, SmallMapList* maps, + CompilationInfo* info, HValue* typecheck = NULL) { HCheckMaps* check_map = new(zone) HCheckMaps(value, zone, typecheck); for (int i = 0; i < maps->length(); i++) { - check_map->Add(maps->at(i), zone); + check_map->Add(maps->at(i), info, zone); } return check_map; } @@ -2668,6 +2683,10 @@ class HCheckMaps V8_FINAL : public HTemplateInstruction<2> { return has_migration_target_; } + bool is_stable() const { + return is_stable_; + } + DECLARE_CONCRETE_INSTRUCTION(CheckMaps) protected: @@ -2678,8 +2697,15 @@ class HCheckMaps V8_FINAL : public HTemplateInstruction<2> { virtual int RedefinedOperandIndex() { return 0; } private: - void Add(Handle map, Zone* zone) { + void Add(Handle map, CompilationInfo* info, Zone* zone) { map_set_.Add(Unique(map), zone); + is_stable_ = is_stable_ && map->is_stable(); + + if (FLAG_check_elimination && is_stable_) { + map->AddDependentCompilationInfo( + DependentCode::kPrototypeCheckGroup, info); + } + if (!has_migration_target_ && map->is_migration_target()) { has_migration_target_ = true; SetChangesFlag(kNewSpacePromotion); @@ -2689,7 +2715,7 @@ class HCheckMaps V8_FINAL : public HTemplateInstruction<2> { // Clients should use one of the static New* methods above. HCheckMaps(HValue* value, Zone *zone, HValue* typecheck) : HTemplateInstruction<2>(value->type()), - omit_(false), has_migration_target_(false) { + omit_(false), has_migration_target_(false), is_stable_(true) { SetOperandAt(0, value); // Use the object value for the dependency if NULL is passed. SetOperandAt(1, typecheck != NULL ? typecheck : value); @@ -2702,6 +2728,7 @@ class HCheckMaps V8_FINAL : public HTemplateInstruction<2> { bool omit_; bool has_migration_target_; + bool is_stable_; UniqueSet map_set_; }; @@ -6477,6 +6504,16 @@ class HStoreNamedField V8_FINAL : public HTemplateInstruction<3> { } SetOperandAt(2, map_constant); has_transition_ = true; + is_stable_ = map->is_stable(); + + if (FLAG_check_elimination && is_stable_) { + map->AddDependentCompilationInfo( + DependentCode::kPrototypeCheckGroup, info); + } + } + + bool is_stable() const { + return is_stable_; } bool NeedsWriteBarrier() { @@ -6515,6 +6552,7 @@ class HStoreNamedField V8_FINAL : public HTemplateInstruction<3> { new_space_dominator_(NULL), write_barrier_mode_(UPDATE_WRITE_BARRIER), has_transition_(false), + is_stable_(false), store_mode_(store_mode) { // Stores to a non existing in-object property are allowed only to the // newly allocated objects (via HAllocate or HInnerAllocatedObject). @@ -6530,6 +6568,7 @@ class HStoreNamedField V8_FINAL : public HTemplateInstruction<3> { HValue* new_space_dominator_; WriteBarrierMode write_barrier_mode_ : 1; bool has_transition_ : 1; + bool is_stable_ : 1; StoreFieldOrKeyedMode store_mode_ : 1; }; diff --git a/src/hydrogen.cc b/src/hydrogen.cc index d33ad92ea5..d3d7b5837a 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -1390,7 +1390,8 @@ HValue* HGraphBuilder::BuildCopyElementsOnWrite(HValue* object, IfBuilder cow_checker(this); - cow_checker.If(elements, factory->fixed_cow_array_map()); + cow_checker.If( + elements, factory->fixed_cow_array_map(), top_info()); cow_checker.Then(); HValue* capacity = AddLoadFixedArrayLength(elements); @@ -1707,7 +1708,7 @@ HValue* HGraphBuilder::BuildNumberToString(HValue* object, Type* type) { // Check if the object is a heap number. IfBuilder if_objectisnumber(this); HValue* objectisnumber = if_objectisnumber.If( - object, isolate()->factory()->heap_number_map()); + object, isolate()->factory()->heap_number_map(), top_info()); if_objectisnumber.Then(); { // Compute hash for heap number similar to double_get_hash(). @@ -5651,13 +5652,15 @@ void HOptimizedGraphBuilder::HandlePolymorphicNamedFieldAccess( HValue* dependency; if (info.type()->Is(Type::Number())) { Handle heap_number_map = isolate()->factory()->heap_number_map(); - compare = New(object, heap_number_map, if_true, if_false); + compare = New(object, heap_number_map, top_info(), + if_true, if_false); dependency = smi_check; } else if (info.type()->Is(Type::String())) { compare = New(object, if_true, if_false); dependency = compare; } else { - compare = New(object, info.map(), if_true, if_false); + compare = New(object, info.map(), top_info(), + if_true, if_false); dependency = compare; } FinishCurrentBlock(compare); @@ -6297,7 +6300,7 @@ HInstruction* HOptimizedGraphBuilder::TryBuildConsolidatedElementLoad( } if (!has_double_maps && !has_smi_or_object_maps) return NULL; - HCheckMaps* checked_object = Add(object, maps); + HCheckMaps* checked_object = Add(object, maps, top_info()); // 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 @@ -6395,7 +6398,7 @@ HValue* HOptimizedGraphBuilder::HandlePolymorphicElementAccess( HBasicBlock* this_map = graph()->CreateBasicBlock(); HBasicBlock* other_map = graph()->CreateBasicBlock(); HCompareMap* mapcompare = - New(object, map, this_map, other_map); + New(object, map, top_info(), this_map, other_map); FinishCurrentBlock(mapcompare); set_current_block(this_map); @@ -6603,7 +6606,7 @@ HInstruction* HOptimizedGraphBuilder::BuildNamedAccess( checked_object = Add(object, HCheckInstanceType::IS_STRING); } else { - checked_object = Add(object, types); + checked_object = Add(object, types, top_info()); } return BuildMonomorphicAccess( &info, object, checked_object, value, ast_id, return_id); @@ -6855,11 +6858,13 @@ void HOptimizedGraphBuilder::HandlePolymorphicCallNamed( Handle map = info.map(); if (info.type()->Is(Type::Number())) { Handle heap_number_map = isolate()->factory()->heap_number_map(); - compare = New(receiver, heap_number_map, if_true, if_false); + compare = New(receiver, heap_number_map, top_info(), + if_true, if_false); } else if (info.type()->Is(Type::String())) { compare = New(receiver, if_true, if_false); } else { - compare = New(receiver, map, if_true, if_false); + compare = New(receiver, map, top_info(), + if_true, if_false); } FinishCurrentBlock(compare); @@ -7704,7 +7709,7 @@ bool HOptimizedGraphBuilder::TryInlineApiCall(Handle function, case kCallApiFunction: case kCallApiMethod: // Need to check that none of the receiver maps could have changed. - Add(receiver, receiver_maps); + Add(receiver, receiver_maps, top_info()); // Need to ensure the chain between receiver and api_holder is intact. if (holder_lookup == CallOptimization::kHolderFound) { AddCheckPrototypeMaps(api_holder, receiver_maps->first()); diff --git a/test/cctest/test-deoptimization.cc b/test/cctest/test-deoptimization.cc index 4b69612f5f..9e36476cba 100644 --- a/test/cctest/test-deoptimization.cc +++ b/test/cctest/test-deoptimization.cc @@ -538,6 +538,7 @@ TEST(DeoptimizeCompare) { TEST(DeoptimizeLoadICStoreIC) { i::FLAG_concurrent_recompilation = false; + i::FLAG_check_elimination = false; LocalContext env; v8::HandleScope scope(env->GetIsolate()); @@ -619,6 +620,7 @@ TEST(DeoptimizeLoadICStoreIC) { TEST(DeoptimizeLoadICStoreICNested) { i::FLAG_concurrent_recompilation = false; + i::FLAG_check_elimination = false; LocalContext env; v8::HandleScope scope(env->GetIsolate()); diff --git a/test/mjsunit/regress/regress-map-invalidation-2.js b/test/mjsunit/regress/regress-map-invalidation-2.js index 1f896a495f..7674e425cb 100644 --- a/test/mjsunit/regress/regress-map-invalidation-2.js +++ b/test/mjsunit/regress/regress-map-invalidation-2.js @@ -47,7 +47,9 @@ function g() { var fun = g(); fun(false, c); fun(false, c); +%OptimizeFunctionOnNextCall(fun); fun(false, c); +%TryMigrateInstance(c); %OptimizeFunctionOnNextCall(fun); fun(false, c); fun(true, c);