From 75432b76960f73fd35ad03d91398bfc0df0edfad Mon Sep 17 00:00:00 2001 From: "verwaest@chromium.org" Date: Wed, 12 Feb 2014 15:38:42 +0000 Subject: [PATCH] Revert "Use stability to only conditionally flush information from the CheckMaps table." R=ishell@chromium.org BUG= Review URL: https://codereview.chromium.org/137863005 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@19331 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, 58 insertions(+), 133 deletions(-) diff --git a/src/code-stubs-hydrogen.cc b/src/code-stubs-hydrogen.cc index 4c646fea08..f9b4d55dfc 100644 --- a/src/code-stubs-hydrogen.cc +++ b/src/code-stubs-hydrogen.cc @@ -295,8 +295,7 @@ 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(), - top_info()); + if_number.OrIf(value, isolate()->factory()->heap_number_map()); if_number.Then(); // Return the number. @@ -359,8 +358,7 @@ HValue* CodeStubGraphBuilder::BuildCodeStub() { HValue* elements = AddLoadElements(boilerplate); IfBuilder if_fixed_cow(this); - if_fixed_cow.If(elements, factory->fixed_cow_array_map(), - top_info()); + if_fixed_cow.If(elements, factory->fixed_cow_array_map()); if_fixed_cow.Then(); push_value = BuildCloneShallowArray(boilerplate, allocation_site, @@ -371,7 +369,7 @@ HValue* CodeStubGraphBuilder::BuildCodeStub() { if_fixed_cow.Else(); IfBuilder if_fixed(this); - if_fixed.If(elements, factory->fixed_array_map(), top_info()); + if_fixed.If(elements, factory->fixed_array_map()); if_fixed.Then(); push_value = BuildCloneShallowArray(boilerplate, allocation_site, diff --git a/src/compiler.cc b/src/compiler.cc index 6ebbe41f9c..b773880b8c 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(": %s]\n", GetBailoutReason(info->bailout_reason())); + PrintF("]\n"); } if (isolate->has_pending_exception()) isolate->clear_pending_exception(); diff --git a/src/hydrogen-check-elimination.cc b/src/hydrogen-check-elimination.cc index d5715a61ec..5fdb3e3f3c 100644 --- a/src/hydrogen-check-elimination.cc +++ b/src/hydrogen-check-elimination.cc @@ -50,7 +50,6 @@ 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_; }; @@ -104,10 +103,9 @@ class HCheckTable : public ZoneObject { } default: { // If the instruction changes maps uncontrollably, drop everything. - if (instr->CheckChangesFlag(kOsrEntries)) { - Reset(); - } else if (instr->CheckChangesFlag(kMaps)) { - KillUnstableEntries(); + if (instr->CheckChangesFlag(kMaps) || + instr->CheckChangesFlag(kOsrEntries)) { + Kill(); } } // Improvements possible: @@ -156,7 +154,6 @@ 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. @@ -178,8 +175,7 @@ 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()), - pred_entry->is_stable_); + copy->Insert(phi, NULL, pred_entry->maps_->Copy(phase_->zone())); } } } @@ -197,13 +193,12 @@ 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(), cmp->is_stable()); + copy->Insert(object, cmp, cmp->map()); } 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)). @@ -222,10 +217,10 @@ class HCheckTable : public ZoneObject { HCheckTableEntry* re = copy->Find(right); if (le == NULL) { if (re != NULL) { - copy->Insert(left, NULL, re->maps_->Copy(zone), re->is_stable_); + copy->Insert(left, NULL, re->maps_->Copy(zone)); } } else if (re == NULL) { - copy->Insert(right, NULL, le->maps_->Copy(zone), le->is_stable_); + copy->Insert(right, NULL, le->maps_->Copy(zone)); } else { MapSet intersect = le->maps_->Intersect(re->maps_, zone); le->maps_ = intersect; @@ -252,7 +247,8 @@ class HCheckTable : public ZoneObject { HBasicBlock* pred_block, Zone* zone) { if (that->size_ == 0) { // If the other state is empty, simply reset. - Reset(); + size_ = 0; + cursor_ = 0; } else { int pred_index = succ->PredecessorIndexOf(pred_block); bool compact = false; @@ -275,8 +271,6 @@ 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; } @@ -357,8 +351,7 @@ class HCheckTable : public ZoneObject { } } else { // No entry; insert a new one. - Insert(object, instr, instr->map_set().Copy(phase_->zone()), - instr->is_stable()); + Insert(object, instr, instr->map_set().Copy(phase_->zone())); } } @@ -420,8 +413,7 @@ class HCheckTable : public ZoneObject { } } else { // No prior information. - // TODO(verwaest): Tag map constants with stability. - Insert(object, instr, map, false); + Insert(object, instr, map); } } @@ -438,14 +430,12 @@ 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()), - instr->is_stable()); + Insert(object, NULL, MapConstant(instr->transition())); } else if (IsMapAccess(instr->access())) { // This is a store directly to the map field of the object. Kill(object); if (!instr->value()->IsConstant()) return; - // TODO(verwaest): Tag with stability. - Insert(object, NULL, MapConstant(instr->value()), false); + Insert(object, NULL, MapConstant(instr->value())); } else { // If the instruction changes maps, it should be handled above. CHECK(!instr->CheckChangesFlag(kMaps)); @@ -495,26 +485,12 @@ class HCheckTable : public ZoneObject { } } - // Reset the table. - void Reset() { + // Kill everything in the table. + void Kill() { 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; @@ -596,24 +572,17 @@ class HCheckTable : public ZoneObject { return entry == NULL ? NULL : entry->maps_; } - void Insert(HValue* object, - HInstruction* check, - Unique map, - bool is_stable) { + void Insert(HValue* object, HInstruction* check, Unique map) { MapSet list = new(phase_->zone()) UniqueSet(); list->Add(map, phase_->zone()); - Insert(object, check, list, is_stable); + Insert(object, check, list); } - void Insert(HValue* object, - HInstruction* check, - MapSet maps, - bool is_stable) { + void Insert(HValue* object, HInstruction* check, MapSet maps) { 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_++; @@ -643,7 +612,8 @@ class HCheckTable : public ZoneObject { class HCheckMapsEffects : public ZoneObject { public: explicit HCheckMapsEffects(Zone* zone) - : stores_(5, zone) { } + : maps_stored_(false), + stores_(5, zone) { } inline bool Disabled() { return false; // Effects are _not_ disabled. @@ -651,22 +621,27 @@ class HCheckMapsEffects : public ZoneObject { // Process a possibly side-effecting instruction. void Process(HInstruction* instr, Zone* zone) { - if (instr->IsStoreNamedField()) { - stores_.Add(HStoreNamedField::cast(instr), zone); - } else { - flags_.Add(instr->ChangesFlags()); + 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)); + } } } // Apply these effects to the given check elimination table. void Apply(HCheckTable* table) { - if (flags_.Contains(kOsrEntries)) { - table->Reset(); - return; - } - if (flags_.Contains(kMaps) || flags_.Contains(kElementsKind)) { + if (maps_stored_) { // Uncontrollable map modifications; kill everything. - table->KillUnstableEntries(); + table->Kill(); return; } @@ -681,14 +656,14 @@ class HCheckMapsEffects : public ZoneObject { // Union these effects with the other effects. void Union(HCheckMapsEffects* that, Zone* zone) { - flags_.Add(that->flags_); + maps_stored_ |= that->maps_stored_; for (int i = 0; i < that->stores_.length(); i++) { stores_.Add(that->stores_[i], zone); } } private: - GVNFlagSet flags_; + bool maps_stored_ : 1; ZoneList stores_; }; @@ -705,7 +680,7 @@ void HCheckEliminationPhase::Run() { } else { // Perform only local analysis. for (int i = 0; i < graph()->blocks()->length(); i++) { - table->Reset(); + table->Kill(); engine.AnalyzeOneBlock(graph()->blocks()->at(i), table); } } diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc index 00087e4041..7a62bb7a42 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, info, zone); + check_map->Add(map, zone); if (map->CanOmitMapChecks() && value->IsConstant() && HConstant::cast(value)->HasMap(map)) { diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h index 20a721273e..63368ce6ea 100644 --- a/src/hydrogen-instructions.h +++ b/src/hydrogen-instructions.h @@ -1508,10 +1508,8 @@ class HBranch V8_FINAL : public HUnaryControlInstruction { class HCompareMap V8_FINAL : public HUnaryControlInstruction { public: - DECLARE_INSTRUCTION_FACTORY_P3(HCompareMap, HValue*, Handle, - CompilationInfo*); - DECLARE_INSTRUCTION_FACTORY_P5(HCompareMap, HValue*, Handle, - CompilationInfo*, + DECLARE_INSTRUCTION_FACTORY_P2(HCompareMap, HValue*, Handle); + DECLARE_INSTRUCTION_FACTORY_P4(HCompareMap, HValue*, Handle, HBasicBlock*, HBasicBlock*); virtual bool KnownSuccessorBlock(HBasicBlock** block) V8_OVERRIDE { @@ -1537,10 +1535,6 @@ class HCompareMap V8_FINAL : public HUnaryControlInstruction { return Representation::Tagged(); } - bool is_stable() const { - return is_stable_; - } - DECLARE_CONCRETE_INSTRUCTION(CompareMap) protected: @@ -1549,22 +1543,14 @@ 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_; }; @@ -2647,11 +2633,10 @@ 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), info, zone); + check_map->Add(maps->at(i), zone); } return check_map; } @@ -2683,10 +2668,6 @@ class HCheckMaps V8_FINAL : public HTemplateInstruction<2> { return has_migration_target_; } - bool is_stable() const { - return is_stable_; - } - DECLARE_CONCRETE_INSTRUCTION(CheckMaps) protected: @@ -2697,15 +2678,8 @@ class HCheckMaps V8_FINAL : public HTemplateInstruction<2> { virtual int RedefinedOperandIndex() { return 0; } private: - void Add(Handle map, CompilationInfo* info, Zone* zone) { + void Add(Handle map, 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); @@ -2715,7 +2689,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), is_stable_(true) { + omit_(false), has_migration_target_(false) { SetOperandAt(0, value); // Use the object value for the dependency if NULL is passed. SetOperandAt(1, typecheck != NULL ? typecheck : value); @@ -2728,7 +2702,6 @@ class HCheckMaps V8_FINAL : public HTemplateInstruction<2> { bool omit_; bool has_migration_target_; - bool is_stable_; UniqueSet map_set_; }; @@ -6504,16 +6477,6 @@ 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() { @@ -6552,7 +6515,6 @@ 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). @@ -6568,7 +6530,6 @@ 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 d3d7b5837a..d33ad92ea5 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -1390,8 +1390,7 @@ HValue* HGraphBuilder::BuildCopyElementsOnWrite(HValue* object, IfBuilder cow_checker(this); - cow_checker.If( - elements, factory->fixed_cow_array_map(), top_info()); + cow_checker.If(elements, factory->fixed_cow_array_map()); cow_checker.Then(); HValue* capacity = AddLoadFixedArrayLength(elements); @@ -1708,7 +1707,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(), top_info()); + object, isolate()->factory()->heap_number_map()); if_objectisnumber.Then(); { // Compute hash for heap number similar to double_get_hash(). @@ -5652,15 +5651,13 @@ 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, top_info(), - if_true, if_false); + compare = New(object, heap_number_map, 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(), top_info(), - if_true, if_false); + compare = New(object, info.map(), if_true, if_false); dependency = compare; } FinishCurrentBlock(compare); @@ -6300,7 +6297,7 @@ HInstruction* HOptimizedGraphBuilder::TryBuildConsolidatedElementLoad( } if (!has_double_maps && !has_smi_or_object_maps) return NULL; - HCheckMaps* checked_object = Add(object, maps, top_info()); + 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 @@ -6398,7 +6395,7 @@ HValue* HOptimizedGraphBuilder::HandlePolymorphicElementAccess( HBasicBlock* this_map = graph()->CreateBasicBlock(); HBasicBlock* other_map = graph()->CreateBasicBlock(); HCompareMap* mapcompare = - New(object, map, top_info(), this_map, other_map); + New(object, map, this_map, other_map); FinishCurrentBlock(mapcompare); set_current_block(this_map); @@ -6606,7 +6603,7 @@ HInstruction* HOptimizedGraphBuilder::BuildNamedAccess( checked_object = Add(object, HCheckInstanceType::IS_STRING); } else { - checked_object = Add(object, types, top_info()); + checked_object = Add(object, types); } return BuildMonomorphicAccess( &info, object, checked_object, value, ast_id, return_id); @@ -6858,13 +6855,11 @@ 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, top_info(), - if_true, if_false); + compare = New(receiver, heap_number_map, if_true, if_false); } else if (info.type()->Is(Type::String())) { compare = New(receiver, if_true, if_false); } else { - compare = New(receiver, map, top_info(), - if_true, if_false); + compare = New(receiver, map, if_true, if_false); } FinishCurrentBlock(compare); @@ -7709,7 +7704,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, top_info()); + Add(receiver, receiver_maps); // 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 9e36476cba..4b69612f5f 100644 --- a/test/cctest/test-deoptimization.cc +++ b/test/cctest/test-deoptimization.cc @@ -538,7 +538,6 @@ TEST(DeoptimizeCompare) { TEST(DeoptimizeLoadICStoreIC) { i::FLAG_concurrent_recompilation = false; - i::FLAG_check_elimination = false; LocalContext env; v8::HandleScope scope(env->GetIsolate()); @@ -620,7 +619,6 @@ 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 7674e425cb..1f896a495f 100644 --- a/test/mjsunit/regress/regress-map-invalidation-2.js +++ b/test/mjsunit/regress/regress-map-invalidation-2.js @@ -47,9 +47,7 @@ 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);