From cc3b7a007fab1d36ed8720152b06c7ed30f6cfe2 Mon Sep 17 00:00:00 2001 From: "mstarzinger@chromium.org" Date: Fri, 9 Aug 2013 15:18:23 +0000 Subject: [PATCH] Allow HPhis to have an invalid merge index. All phis that do not represent local variables or values on the operand stack are not allowed to carry a merge index, as the replay of the HEnvironment during LChunkBuilder time might get out of sync due to colliding indexes. R=danno@chromium.org BUG=v8:2815 Review URL: https://codereview.chromium.org/22494003 git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@16135 ce2b1a6d-e550-0410-aec6-3dcde31c8c00 --- src/arm/lithium-arm.cc | 6 ++++- src/hydrogen-dce.cc | 4 ++- src/hydrogen-escape-analysis.cc | 7 ++---- src/hydrogen-instructions.h | 6 ++++- src/hydrogen-osr.cc | 1 + src/hydrogen.cc | 36 ++++++++++++++++----------- src/hydrogen.h | 54 +++++++++++++++++++---------------------- src/ia32/lithium-ia32.cc | 6 ++++- src/mips/lithium-mips.cc | 6 ++++- src/x64/lithium-x64.cc | 6 ++++- 10 files changed, 78 insertions(+), 54 deletions(-) diff --git a/src/arm/lithium-arm.cc b/src/arm/lithium-arm.cc index cd8d994..6060ef2 100644 --- a/src/arm/lithium-arm.cc +++ b/src/arm/lithium-arm.cc @@ -815,7 +815,11 @@ void LChunkBuilder::DoBasicBlock(HBasicBlock* block, HBasicBlock* next_block) { HEnvironment* last_environment = pred->last_environment(); for (int i = 0; i < block->phis()->length(); ++i) { HPhi* phi = block->phis()->at(i); - if (phi->merged_index() < last_environment->length()) { + // TODO(mstarzinger): The length check below should actually not + // be necessary, but some array stubs already rely on it. This + // should be investigated and fixed. + if (phi->HasMergedIndex() && + phi->merged_index() < last_environment->length()) { last_environment->SetValueAt(phi->merged_index(), phi); } } diff --git a/src/hydrogen-dce.cc b/src/hydrogen-dce.cc index 4ad32d2..0e7253d 100644 --- a/src/hydrogen-dce.cc +++ b/src/hydrogen-dce.cc @@ -118,7 +118,9 @@ void HDeadCodeEliminationPhase::RemoveDeadInstructions() { HPhi* phi = worklist.RemoveLast(); HBasicBlock* block = phi->block(); phi->DeleteAndReplaceWith(NULL); - block->RecordDeletedPhi(phi->merged_index()); + if (phi->HasMergedIndex()) { + block->RecordDeletedPhi(phi->merged_index()); + } } } diff --git a/src/hydrogen-escape-analysis.cc b/src/hydrogen-escape-analysis.cc index 5eaa88b..3300f0b 100644 --- a/src/hydrogen-escape-analysis.cc +++ b/src/hydrogen-escape-analysis.cc @@ -110,14 +110,11 @@ HCapturedObject* HEscapeAnalysisPhase::NewStateCopy( // Insert a newly created phi into the given block and fill all incoming -// edges with the given value. The merge index is chosen so that it is -// unique for this particular scalar replacement index. +// edges with the given value. HPhi* HEscapeAnalysisPhase::NewPhiAndInsert( HBasicBlock* block, HValue* incoming_value, int index) { Zone* zone = graph()->zone(); - HBasicBlock* pred = block->predecessors()->first(); - int phi_index = pred->last_environment()->length() + cumulative_values_; - HPhi* phi = new(zone) HPhi(phi_index + index, zone); + HPhi* phi = new(zone) HPhi(HPhi::kInvalidMergedIndex, zone); for (int i = 0; i < block->predecessors()->length(); i++) { phi->AddInput(incoming_value); } diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h index 85bfc8f..815a6c6 100644 --- a/src/hydrogen-instructions.h +++ b/src/hydrogen-instructions.h @@ -3042,7 +3042,7 @@ class HPhi: public HValue { non_phi_uses_[i] = 0; indirect_uses_[i] = 0; } - ASSERT(merged_index >= 0); + ASSERT(merged_index >= 0 || merged_index == kInvalidMergedIndex); SetFlag(kFlexibleRepresentation); SetFlag(kAllowUndefinedAsNaN); } @@ -3065,6 +3065,7 @@ class HPhi: public HValue { bool HasRealUses(); bool IsReceiver() const { return merged_index_ == 0; } + bool HasMergedIndex() const { return merged_index_ != kInvalidMergedIndex; } int merged_index() const { return merged_index_; } @@ -3127,6 +3128,9 @@ class HPhi: public HValue { void SimplifyConstantInputs(); + // Marker value representing an invalid merge index. + static const int kInvalidMergedIndex = -1; + protected: virtual void DeleteFromGraph(); virtual void InternalSetOperandAt(int index, HValue* value) { diff --git a/src/hydrogen-osr.cc b/src/hydrogen-osr.cc index 6c3d6ae..73fa40a 100644 --- a/src/hydrogen-osr.cc +++ b/src/hydrogen-osr.cc @@ -117,6 +117,7 @@ void HOsrBuilder::FinishOsrValues() { const ZoneList* phis = osr_loop_entry_->phis(); for (int j = 0; j < phis->length(); j++) { HPhi* phi = phis->at(j); + ASSERT(phi->HasMergedIndex()); osr_values_->at(phi->merged_index())->set_incoming_value(phi); } } diff --git a/src/hydrogen.cc b/src/hydrogen.cc index a05298d..e5af9f8 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -148,6 +148,16 @@ void HBasicBlock::AddInstruction(HInstruction* instr) { } +HPhi* HBasicBlock::AddNewPhi(int merged_index) { + if (graph()->IsInsideNoSideEffectsScope()) { + merged_index = HPhi::kInvalidMergedIndex; + } + HPhi* phi = new(zone()) HPhi(merged_index, zone()); + AddPhi(phi); + return phi; +} + + HSimulate* HBasicBlock::CreateSimulate(BailoutId ast_id, RemovableSimulate removable) { ASSERT(HasEnvironment()); @@ -203,7 +213,7 @@ void HBasicBlock::Goto(HBasicBlock* block, UpdateEnvironment(last_environment()->DiscardInlined(drop_extra)); } - if (add_simulate) AddSimulate(BailoutId::None()); + if (add_simulate) AddNewSimulate(BailoutId::None()); HGoto* instr = new(zone()) HGoto(block); Finish(instr); } @@ -219,7 +229,7 @@ void HBasicBlock::AddLeaveInlined(HValue* return_value, AddInstruction(new(zone()) HLeaveInlined()); UpdateEnvironment(last_environment()->DiscardInlined(drop_extra)); last_environment()->Push(return_value); - AddSimulate(BailoutId::None()); + AddNewSimulate(BailoutId::None()); HGoto* instr = new(zone()) HGoto(target); Finish(instr); } @@ -904,8 +914,7 @@ HValue* HGraphBuilder::LoopBuilder::BeginBody( HValue* terminating, Token::Value token) { HEnvironment* env = builder_->environment(); - phi_ = new(zone()) HPhi(env->values()->length(), zone()); - header_block_->AddPhi(phi_); + phi_ = header_block_->AddNewPhi(env->values()->length()); phi_->AddInput(initial); env->Push(initial); builder_->current_block()->GotoNoSimulate(header_block_); @@ -982,7 +991,7 @@ HGraph* HGraphBuilder::CreateGraph() { HInstruction* HGraphBuilder::AddInstruction(HInstruction* instr) { ASSERT(current_block() != NULL); current_block()->AddInstruction(instr); - if (no_side_effects_scope_count_ > 0) { + if (graph()->IsInsideNoSideEffectsScope()) { instr->SetFlag(HValue::kHasNoObservableSideEffects); } return instr; @@ -1006,8 +1015,8 @@ void HGraphBuilder::AddIncrementCounter(StatsCounter* counter, void HGraphBuilder::AddSimulate(BailoutId id, RemovableSimulate removable) { ASSERT(current_block() != NULL); - ASSERT(no_side_effects_scope_count_ == 0); - current_block()->AddSimulate(id, removable); + ASSERT(!graph()->IsInsideNoSideEffectsScope()); + current_block()->AddNewSimulate(id, removable); } @@ -1037,7 +1046,7 @@ void HGraphBuilder::FinishExitWithHardDeoptimization( const char* reason, HBasicBlock* continuation) { PadEnvironmentForContinuation(current_block(), continuation); Add(reason, Deoptimizer::EAGER); - if (no_side_effects_scope_count_ > 0) { + if (graph()->IsInsideNoSideEffectsScope()) { current_block()->GotoNoSimulate(continuation); } else { current_block()->Goto(continuation); @@ -2049,7 +2058,8 @@ HGraph::HGraph(CompilationInfo* info) has_soft_deoptimize_(false), depends_on_empty_array_proto_elements_(false), type_change_checksum_(0), - maximum_environment_size_(0) { + maximum_environment_size_(0), + no_side_effects_scope_count_(0) { if (info->IsStub()) { HydrogenCodeStub* stub = info->code_stub(); CodeStubInterfaceDescriptor* descriptor = @@ -9326,20 +9336,19 @@ void HEnvironment::AddIncomingEdge(HBasicBlock* block, HEnvironment* other) { // There is already a phi for the i'th value. HPhi* phi = HPhi::cast(value); // Assert index is correct and that we haven't missed an incoming edge. - ASSERT(phi->merged_index() == i); + ASSERT(phi->merged_index() == i || !phi->HasMergedIndex()); ASSERT(phi->OperandCount() == block->predecessors()->length()); phi->AddInput(other->values_[i]); } else if (values_[i] != other->values_[i]) { // There is a fresh value on the incoming edge, a phi is needed. ASSERT(values_[i] != NULL && other->values_[i] != NULL); - HPhi* phi = new(zone()) HPhi(i, zone()); + HPhi* phi = block->AddNewPhi(i); HValue* old_value = values_[i]; for (int j = 0; j < block->predecessors()->length(); j++) { phi->AddInput(old_value); } phi->AddInput(other->values_[i]); this->values_[i] = phi; - block->AddPhi(phi); } } } @@ -9400,10 +9409,9 @@ HEnvironment* HEnvironment::CopyWithoutHistory() const { HEnvironment* HEnvironment::CopyAsLoopHeader(HBasicBlock* loop_header) const { HEnvironment* new_env = Copy(); for (int i = 0; i < values_.length(); ++i) { - HPhi* phi = new(zone()) HPhi(i, zone()); + HPhi* phi = loop_header->AddNewPhi(i); phi->AddInput(values_[i]); new_env->values_[i] = phi; - loop_header->AddPhi(phi); } new_env->ClearHistory(); return new_env; diff --git a/src/hydrogen.h b/src/hydrogen.h index d5c6040..9c1d766 100644 --- a/src/hydrogen.h +++ b/src/hydrogen.h @@ -142,8 +142,9 @@ class HBasicBlock: public ZoneObject { } int PredecessorIndexOf(HBasicBlock* predecessor) const; - HSimulate* AddSimulate(BailoutId ast_id, - RemovableSimulate removable = FIXED_SIMULATE) { + HPhi* AddNewPhi(int merged_index); + HSimulate* AddNewSimulate(BailoutId ast_id, + RemovableSimulate removable = FIXED_SIMULATE) { HSimulate* instr = CreateSimulate(ast_id, removable); AddInstruction(instr); return instr; @@ -453,6 +454,10 @@ class HGraph: public ZoneObject { uint32_instructions_->Add(instr, zone()); } + void IncrementInNoSideEffectsScope() { no_side_effects_scope_count_++; } + void DecrementInNoSideEffectsScope() { no_side_effects_scope_count_--; } + bool IsInsideNoSideEffectsScope() { return no_side_effects_scope_count_ > 0; } + private: HConstant* GetConstant(SetOncePointer* pointer, int32_t integer_value); @@ -498,6 +503,7 @@ class HGraph: public ZoneObject { bool depends_on_empty_array_proto_elements_; int type_change_checksum_; int maximum_environment_size_; + int no_side_effects_scope_count_; DISALLOW_COPY_AND_ASSIGN(HGraph); }; @@ -970,8 +976,7 @@ class HGraphBuilder { explicit HGraphBuilder(CompilationInfo* info) : info_(info), graph_(NULL), - current_block_(NULL), - no_side_effects_scope_count_(0) {} + current_block_(NULL) {} virtual ~HGraphBuilder() {} HBasicBlock* current_block() const { return current_block_; } @@ -1186,16 +1191,7 @@ class HGraphBuilder { AddInstruction(NewUncasted(p1, p2, p3, p4, p5, p6, p7, p8))); } - void AddSimulate(BailoutId id, - RemovableSimulate removable = FIXED_SIMULATE); - - void IncrementInNoSideEffectsScope() { - no_side_effects_scope_count_++; - } - - void DecrementInNoSideEffectsScope() { - no_side_effects_scope_count_--; - } + void AddSimulate(BailoutId id, RemovableSimulate removable = FIXED_SIMULATE); protected: virtual bool BuildGraph() = 0; @@ -1442,20 +1438,6 @@ class HGraphBuilder { bool finished_; }; - class NoObservableSideEffectsScope { - public: - explicit NoObservableSideEffectsScope(HGraphBuilder* builder) : - builder_(builder) { - builder_->IncrementInNoSideEffectsScope(); - } - ~NoObservableSideEffectsScope() { - builder_->DecrementInNoSideEffectsScope(); - } - - private: - HGraphBuilder* builder_; - }; - HValue* BuildNewElementsCapacity(HValue* old_capacity); void BuildNewSpaceArrayCheck(HValue* length, @@ -1577,7 +1559,6 @@ class HGraphBuilder { CompilationInfo* info_; HGraph* graph_; HBasicBlock* current_block_; - int no_side_effects_scope_count_; }; @@ -2330,6 +2311,21 @@ class HTracer: public Malloced { }; +class NoObservableSideEffectsScope { + public: + explicit NoObservableSideEffectsScope(HGraphBuilder* builder) : + builder_(builder) { + builder_->graph()->IncrementInNoSideEffectsScope(); + } + ~NoObservableSideEffectsScope() { + builder_->graph()->DecrementInNoSideEffectsScope(); + } + + private: + HGraphBuilder* builder_; +}; + + } } // namespace v8::internal #endif // V8_HYDROGEN_H_ diff --git a/src/ia32/lithium-ia32.cc b/src/ia32/lithium-ia32.cc index cce7f9f..323cd72 100644 --- a/src/ia32/lithium-ia32.cc +++ b/src/ia32/lithium-ia32.cc @@ -870,7 +870,11 @@ void LChunkBuilder::DoBasicBlock(HBasicBlock* block, HBasicBlock* next_block) { HEnvironment* last_environment = pred->last_environment(); for (int i = 0; i < block->phis()->length(); ++i) { HPhi* phi = block->phis()->at(i); - if (phi->merged_index() < last_environment->length()) { + // TODO(mstarzinger): The length check below should actually not + // be necessary, but some array stubs already rely on it. This + // should be investigated and fixed. + if (phi->HasMergedIndex() && + phi->merged_index() < last_environment->length()) { last_environment->SetValueAt(phi->merged_index(), phi); } } diff --git a/src/mips/lithium-mips.cc b/src/mips/lithium-mips.cc index a054168..09ec478 100644 --- a/src/mips/lithium-mips.cc +++ b/src/mips/lithium-mips.cc @@ -820,7 +820,11 @@ void LChunkBuilder::DoBasicBlock(HBasicBlock* block, HBasicBlock* next_block) { HEnvironment* last_environment = pred->last_environment(); for (int i = 0; i < block->phis()->length(); ++i) { HPhi* phi = block->phis()->at(i); - if (phi->merged_index() < last_environment->length()) { + // TODO(mstarzinger): The length check below should actually not + // be necessary, but some array stubs already rely on it. This + // should be investigated and fixed. + if (phi->HasMergedIndex() && + phi->merged_index() < last_environment->length()) { last_environment->SetValueAt(phi->merged_index(), phi); } } diff --git a/src/x64/lithium-x64.cc b/src/x64/lithium-x64.cc index b339388..ac372b6 100644 --- a/src/x64/lithium-x64.cc +++ b/src/x64/lithium-x64.cc @@ -814,7 +814,11 @@ void LChunkBuilder::DoBasicBlock(HBasicBlock* block, HBasicBlock* next_block) { HEnvironment* last_environment = pred->last_environment(); for (int i = 0; i < block->phis()->length(); ++i) { HPhi* phi = block->phis()->at(i); - if (phi->merged_index() < last_environment->length()) { + // TODO(mstarzinger): The length check below should actually not + // be necessary, but some array stubs already rely on it. This + // should be investigated and fixed. + if (phi->HasMergedIndex() && + phi->merged_index() < last_environment->length()) { last_environment->SetValueAt(phi->merged_index(), phi); } } -- 2.7.4