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

diff --git a/src/arm/lithium-arm.cc b/src/arm/lithium-arm.cc
index cd8d994..6060ef2 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- a/src/hydrogen-osr.cc
+++ b/src/hydrogen-osr.cc
@@ -117,6 +117,7 @@ void HOsrBuilder::FinishOsrValues() {
   const ZoneList<HPhi*>* 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 (file)
--- 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<HDeoptimize>(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 (file)
--- 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<HConstant>* 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<I>(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 (file)
--- 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 (file)
--- 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 (file)
--- 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);
       }
     }