Remove dead phis as soon as they are discovered.

runtime/vm/flow_graph_allocator.cc

Index: runtime/vm/flow_graph_allocator.cc
diff --git a/runtime/vm/flow_graph_allocator.cc b/runtime/vm/flow_graph_allocator.cc
index 1c77f38b6a0a6a6955d1da755888c6f63ebdc0ef..a346b9dff979146f099219000b7b6750df62f01c 100644
--- a/runtime/vm/flow_graph_allocator.cc
+++ b/runtime/vm/flow_graph_allocator.cc
@@ -91,37 +91,12 @@ FlowGraphAllocator::FlowGraphAllocator(const FlowGraph& flow_graph)
 
 
 // Remove environments from the instructions which can't deoptimize.
-// Replace dead phis uses with null values in environments.
-void FlowGraphAllocator::EliminateEnvironmentUses() {
-  ConstantInstr* constant_null =
-      postorder_.Last()->AsGraphEntry()->constant_null();
+void FlowGraphAllocator::EliminateEnvironments() {
   for (intptr_t i = 0; i < block_order_.length(); ++i) {
     BlockEntryInstr* block = block_order_[i];
-    if (block->IsJoinEntry()) block->AsJoinEntry()->RemoveDeadPhis();
     for (ForwardInstructionIterator it(block); !it.Done(); it.Advance()) {
       Instruction* current = it.Current();
-      if (current->CanDeoptimize()) {
-        ASSERT(current->env() != NULL);
-        for (Environment::DeepIterator it(current->env());
-             !it.Done();
-             it.Advance()) {
-          Value* use = it.CurrentValue();
-          Definition* def = use->definition();
-          PushArgumentInstr* push_argument = def->AsPushArgument();
-          if ((push_argument != NULL) && push_argument->WasEliminated()) {
-            it.SetCurrentValue(push_argument->value()->Copy());

[Vyacheslav Egorov (Google), 2013/02/27 21:53:02]

What happened to this special PushArgument handling? Are we relying on the fact
that push argument removal does this substitution? Can we assert that
environment contains no PushArguments.

[Kevin Millikin (Google), 2013/02/28 08:12:02]

It was already obsolete after a previous change, where they were removed eagerly
from environments.

We can add an assert, but it seemed to me that it was just asserting a negative
property of the old implementation.  

Presumably, any removed value turning up in an environment would end up with an
unallocated value and an assertion failure later.  So we can actually assert a
stronger property here if we want, or rely on the later assert.

-            continue;
-          }
-
-          PhiInstr* phi = def->AsPhi();
-          if ((phi != NULL) && !phi->is_alive()) {
-            it.SetCurrentValue(new Value(constant_null));
-            continue;
-          }
-        }
-      } else {
-        current->RemoveEnvironment();
-      }
+      if (!current->CanDeoptimize()) current->RemoveEnvironment();
     }
   }
 }
@@ -177,25 +152,22 @@ void FlowGraphAllocator::ComputeInitialSets() {
     // Handle phis.
     if (block->IsJoinEntry()) {
       JoinEntryInstr* join = block->AsJoinEntry();
-      if (join->phis() != NULL) {
-        for (intptr_t j = 0; j < join->phis()->length(); j++) {
-          PhiInstr* phi = (*join->phis())[j];
-          if (phi == NULL) continue;
-
-          kill->Add(phi->ssa_temp_index());
-          live_in->Remove(phi->ssa_temp_index());
-
-          // If phi-operand is not defined by a predecessor it must be marked
-          // live-in for a predecessor.
-          for (intptr_t k = 0; k < phi->InputCount(); k++) {
-            Value* val = phi->InputAt(k);
-            if (val->BindsToConstant()) continue;
-
-            BlockEntryInstr* pred = block->PredecessorAt(k);
-            const intptr_t use = val->definition()->ssa_temp_index();
-            if (!kill_[pred->postorder_number()]->Contains(use)) {
-              live_in_[pred->postorder_number()]->Add(use);
-            }
+      for (PhiIterator it(join); !it.Done(); it.Advance()) {
+        PhiInstr* phi = it.Current();
+        ASSERT(phi != NULL);
+        kill->Add(phi->ssa_temp_index());
+        live_in->Remove(phi->ssa_temp_index());
+
+        // If a phi input is not defined by the corresponding predecessor it
+        // must be marked live-in for that predecessor.
+        for (intptr_t k = 0; k < phi->InputCount(); k++) {
+          Value* val = phi->InputAt(k);
+          if (val->BindsToConstant()) continue;
+
+          BlockEntryInstr* pred = block->PredecessorAt(k);
+          const intptr_t use = val->definition()->ssa_temp_index();
+          if (!kill_[pred->postorder_number()]->Contains(use)) {
+            live_in_[pred->postorder_number()]->Add(use);
           }
         }
       }
@@ -696,12 +668,9 @@ Instruction* FlowGraphAllocator::ConnectOutgoingPhiMoves(
   const intptr_t pred_idx = join->IndexOfPredecessor(block);
 
   // Record the corresponding phi input use for each phi.
-  ZoneGrowableArray<PhiInstr*>* phis = join->phis();
   intptr_t move_idx = 0;
-  for (intptr_t phi_idx = 0; phi_idx < phis->length(); phi_idx++) {
-    PhiInstr* phi = (*phis)[phi_idx];
-    if (phi == NULL) continue;
-
+  for (PhiIterator it(join); !it.Done(); it.Advance()) {
+    PhiInstr* phi = it.Current();
     Value* val = phi->InputAt(pred_idx);
     MoveOperands* move = parallel_move->MoveOperandsAt(move_idx);
 
@@ -744,19 +713,13 @@ void FlowGraphAllocator::ConnectIncomingPhiMoves(BlockEntryInstr* block) {
 
   // All uses are recorded at the start position in the block.
   const intptr_t pos = join->start_pos();
-
-  ZoneGrowableArray<PhiInstr*>* phis = join->phis();
-  if (phis == NULL) return;
-
   const bool is_loop_header = BlockInfoAt(join->start_pos())->is_loop_header();
-
   intptr_t move_idx = 0;
-  for (intptr_t phi_idx = 0; phi_idx < phis->length(); phi_idx++) {
-    PhiInstr* phi = (*phis)[phi_idx];
-    if (phi == NULL) continue;
-
+  for (PhiIterator it(join); !it.Done(); it.Advance()) {
+    PhiInstr* phi = it.Current();
+    ASSERT(phi != NULL);
     const intptr_t vreg = phi->ssa_temp_index();
-    ASSERT(vreg != -1);
+    ASSERT(vreg >= 0);
 
     // Expected shape of live range:
     //
@@ -1183,8 +1146,10 @@ void FlowGraphAllocator::NumberInstructions() {
     // For join entry predecessors create phi resolution moves if
     // necessary. They will be populated by the register allocator.
     JoinEntryInstr* join = block->AsJoinEntry();
-    if ((join != NULL) && (join->phi_count() > 0)) {
-      const intptr_t phi_count = join->phi_count();
+    if ((join != NULL) &&
+        (join->phis() != NULL) &&
+        !join->phis()->is_empty()) {
+      const intptr_t phi_count = join->phis()->length();
       for (intptr_t i = 0; i < block->PredecessorCount(); i++) {
         // Insert the move between the last two instructions of the
         // predecessor block (all such blocks have at least two instructions:
@@ -1756,15 +1721,14 @@ bool FlowGraphAllocator::AllocateFreeRegister(LiveRange* unallocated) {
          !it.Done();
          it.Advance()) {
       PhiInstr* phi = it.Current();
-      if (phi->is_alive()) {
-        const intptr_t phi_vreg = phi->ssa_temp_index();
-        LiveRange* range = GetLiveRange(phi_vreg);
-        if (range->assigned_location().kind() == register_kind_) {
-          const intptr_t reg = range->assigned_location().register_code();
-
-          if (!reaching_defs_.Get(phi)->Contains(unallocated->vreg())) {
-            used_on_backedge[reg] = true;
-          }
+      ASSERT(phi->is_alive());
+      const intptr_t phi_vreg = phi->ssa_temp_index();
+      LiveRange* range = GetLiveRange(phi_vreg);
+      if (range->assigned_location().kind() == register_kind_) {
+        const intptr_t reg = range->assigned_location().register_code();
+
+        if (!reaching_defs_.Get(phi)->Contains(unallocated->vreg())) {
+          used_on_backedge[reg] = true;
         }
       }
     }
@@ -2446,7 +2410,7 @@ void FlowGraphAllocator::CollectRepresentations() {
 void FlowGraphAllocator::AllocateRegisters() {
   CollectRepresentations();
 
-  EliminateEnvironmentUses();
+  EliminateEnvironments();
 
   AnalyzeLiveness();