Optimize functions containing try-catch.

runtime/vm/flow_graph.cc

Index: runtime/vm/flow_graph.cc
===================================================================
--- runtime/vm/flow_graph.cc	(revision 22436)
+++ runtime/vm/flow_graph.cc	(working copy)
@@ -15,6 +15,12 @@
 DECLARE_FLAG(bool, trace_optimization);
 DECLARE_FLAG(bool, verify_compiler);
 
+#if defined(TARGET_ARCH_ARM) || defined(TARGET_ARCH_MIPS)
+DEFINE_FLAG(bool, optimize_try_catch, false, "Optimization of try-catch");
+#else
+DEFINE_FLAG(bool, optimize_try_catch, true, "Optimization of try-catch");
+#endif
+
 FlowGraph::FlowGraph(const FlowGraphBuilder& builder,
                      GraphEntryInstr* graph_entry,
                      intptr_t max_block_id)
@@ -327,8 +333,13 @@
     const intptr_t block_count = flow_graph_->preorder().length();
     for (intptr_t i = 0; i < block_count; i++) {
       BlockEntryInstr* block = flow_graph_->preorder()[i];
+      // All locals are assigned inside try-catch.

[Kevin Millikin (Google), 2013/05/08 11:42:00]

Comment needs to say why.  I don't understand why it must be the case that all
locals are assigned in every block inside a try.

Also: be precise in the comments about the difference between blocks inside the
try, blocks inside the catch, the try entry, and the catch entry.  It can be
ambiguous to say simply 'inside try-catch'.

[Florian Schneider, 2013/05/08 17:10:55]

Done.

       BitVector* kill = GetKillSet(block);
-      kill->Intersect(GetLiveOutSet(block));
+      if (block->try_index() != CatchClauseNode::kInvalidTryIndex) {
+        kill->SetAll();
+      } else {
+        kill->Intersect(GetLiveOutSet(block));
+      }
       assigned_vars_.Add(kill);
     }
 
@@ -377,6 +388,12 @@
     BitVector* live_in = live_in_[i];
     last_loads->Clear();
 
+    // Inside try-catch all locals are live.

[Kevin Millikin (Google), 2013/05/08 11:42:00]

Again, I don't understand why.  This is just for all the blocks occurring inside
the try, right?

Is this an approximation because we don't have edges from the calls to the catch
block?

[Florian Schneider, 2013/05/08 17:10:55]

Done.

+    if (block->try_index() != CatchClauseNode::kInvalidTryIndex) {
+      live_in->SetAll();
+      continue;
+    }
+
     // Iterate backwards starting at the last instruction.
     for (BackwardInstructionIterator it(block); !it.Done(); it.Advance()) {
       Instruction* current = it.Current();
@@ -385,7 +402,7 @@
       if (load != NULL) {
         const intptr_t index = load->local().BitIndexIn(num_non_copied_params_);
         live_in->Add(index);
-        if (!last_loads->Contains(index)) {
+        if (!last_loads->Contains(index) && !load->local().always_live()) {
           last_loads->Add(index);
           load->mark_last();
         }
@@ -393,7 +410,7 @@
       }
 
       StoreLocalInstr* store = current->AsStoreLocal();
-      if (store != NULL) {
+      if (store != NULL && !store->local().always_live()) {
         const intptr_t index =
             store->local().BitIndexIn(num_non_copied_params_);
         if (kill->Contains(index)) {
@@ -406,7 +423,9 @@
           }
           kill->Add(index);
         }
-        live_in->Remove(index);
+        if (block->try_index() == CatchClauseNode::kInvalidTryIndex) {

[Kevin Millikin (Google), 2013/05/08 11:42:00]

This comparison is always true.  The loop is not entered in the case that the
condition is false.

[Florian Schneider, 2013/05/08 17:10:55]

Done.

+          live_in->Remove(index);
+        }
         continue;
       }
     }
@@ -616,8 +635,7 @@
 void FlowGraph::Rename(GrowableArray<PhiInstr*>* live_phis,
                        VariableLivenessAnalysis* variable_liveness,
                        ZoneGrowableArray<Definition*>* inlining_parameters) {
-  // TODO(fschneider): Support catch-entry.
-  if (graph_entry_->SuccessorCount() > 1) {
+  if (!FLAG_optimize_try_catch && (graph_entry_->SuccessorCount() > 1)) {
     Bailout("Catch-entry support in SSA.");
   }
 
@@ -653,9 +671,14 @@
     env.Add(constant_null());
   }
 
-  BlockEntryInstr* normal_entry = graph_entry_->SuccessorAt(0);
-  ASSERT(normal_entry != NULL);  // Must have entry.
-  RenameRecursive(normal_entry, &env, live_phis, variable_liveness);
+  if (graph_entry_->SuccessorCount() > 1) {
+    // Functions with try-catch have a fixed area of stack slots reserved
+    // so that all local variables are stored at a known location when
+    // on entry to the catch.
+    graph_entry_->set_fixed_slot_count(
+        num_stack_locals() + num_copied_params());
+  }
+  RenameRecursive(graph_entry_, &env, live_phis, variable_liveness);
 }
 
 
@@ -689,9 +712,22 @@
         if (phi != NULL) {
           (*env)[i] = phi;
           phi->set_ssa_temp_index(alloc_ssa_temp_index());  // New SSA temp.
+          if (block_entry->try_index() != CatchClauseNode::kInvalidTryIndex) {

[Kevin Millikin (Google), 2013/05/08 11:42:00]

This comparison is repeated a lot in this change.  It's more readable to
introduce a predicate on blocks IsInsideTry, or NestedInsideTry, or the like.

We also need the comment to record why all phis inside blocks inside the try are
always live, even if not otherwise.

[Florian Schneider, 2013/05/08 17:10:55]

Done.

+            phi->mark_alive();
+            live_phis->Add(phi);
+          }
         }
       }
     }
+  } else if (block_entry->IsCatchBlockEntry()) {
+    // Add real definitions for all locals and parameters. The location of the
+    // parameters and locals is the same as at function entry.

[Kevin Millikin (Google), 2013/05/08 11:42:00]

I don't think this comment is quite correct.  The locals are all uninitialized
at function entry and do not have distinct locations, do they?

I think you mean that the location is the same as in the unoptimized stack
frame, though I'm not sure that's important here.  Nothing about this place has
anything to do with locations anyway.

[Florian Schneider, 2013/05/08 17:10:55]

Done.

+    for (intptr_t i = 0; i < env->length(); ++i) {
+      ParameterInstr* param = new ParameterInstr(i, block_entry);
+      param->set_ssa_temp_index(alloc_ssa_temp_index());  // New SSA temp.
+      (*env)[i] = param;
+      block_entry->AsCatchBlockEntry()->initial_definitions()->Add(param);
+    }
   }
 
   // Attach environment to the block entry.
@@ -750,7 +786,8 @@
           index = store->local().BitIndexIn(num_non_copied_params_);
           result = store->value()->definition();
 
-          if (variable_liveness->IsStoreAlive(block_entry, store)) {
+          if (variable_liveness->IsStoreAlive(block_entry, store) ||
+              store->local().always_live()) {

[Kevin Millikin (Google), 2013/05/08 11:42:00]

Need to know why, also below.

These are for the the three special catch variables, right?  Do we have stores
to them that have implicit loads (so they aren't reflected in liveness analysis)
and should not be eliminated?

[Florian Schneider, 2013/05/08 17:10:55]

Done. Not needed anymore.

             (*env)[index] = result;
           } else {
             (*env)[index] = constant_null();
@@ -768,7 +805,8 @@
             live_phis->Add(phi);
           }
 
-          if (variable_liveness->IsLastLoad(block_entry, load)) {
+          if (variable_liveness->IsLastLoad(block_entry, load) &&
+              !load->local().always_live()) {

[Kevin Millikin (Google), 2013/05/08 11:42:00]

Is this because there are implicit loads from the special catch variables, so
this might not really be the last load?

[Florian Schneider, 2013/05/08 17:10:55]

Done. Not needed anymore.

             (*env)[index] = constant_null();
           }
         }