dart2dart: Bugfix in copy propagator.

pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 part of tree_ir.optimization;
 
 /// Eliminates moving assignments, such as w := v, by assigning directly to w
 /// at the definition of v.
 ///
 /// This compensates for suboptimal register allocation, and merges closure
 /// variables with local temporaries that were left behind when translating
 /// out of CPS (where closure variables live in a separate space).
 class CopyPropagator extends RecursiveVisitor with PassMixin {
 
   /// After visitStatement returns, [move] maps a variable v to an
   /// assignment A of form w := v, under the following conditions:
   /// - there are no reads or writes of w before A
   /// - A is the only use of v
   Map<Variable, Assign> move = <Variable, Assign>{};
 
   /// Like [move], except w is the key instead of v.
   Map<Variable, Assign> inverseMove = <Variable, Assign>{};
 
   ExecutableElement currentElement;
 
+  /// Number of try blocks enclosing the currently visited node.
+  int enclosingTrys = 0;
+
   void rewriteExecutableDefinition(ExecutableDefinition root) {
     currentElement = root.element;
     root.body = visitStatement(root.body);
   }
 
   rewriteFunctionDefinition(FunctionDefinition node) {
     if (node.isAbstract) return;
     rewriteExecutableDefinition(node);
 
     // Try to propagate moving assignments into function parameters.
@@ skipping 125 matching lines @@
     node.variable = copyPropagateVariable(node.variable);
 
     // If a moving assignment w := v exists later, and we assign to w here,
     // the moving assignment is no longer a candidate for copy propagation.
     invalidateMovingAssignment(node.variable);
 
     visitExpression(node.definition);
 
     // If this is a moving assignment w := v, with this being the only use of v,
     // try to propagate it backwards.  Do not propagate assignments where w
-    // is from an outer function scope.
-    if (node.definition is VariableUse) {
+    // is captured or if where are inside a try block, because then we can't
+    // isolate the uses of w to a given basic block.
+    if (node.definition is VariableUse && !node.isDeclaration &&

[Kevin Millikin (Google), 2015/03/06 09:30:33]

I think it's confusing to call this field Assign.definition.  Maybe .value,
.expression, or .rightHandSide?

.isDeclaration means that it is the initial assignment to 'w', right?  It's not
immediately clear why we can't propagate that backward (is it just because the
.isDeclaration flag is not updated in that case?).  Maybe declaration isn't
quite right, because declarations are hoisted.

Finally, with this many conjuncts, I prefer to have them one per line.  It eases
(my) understanding (small ones are easy to miss, it's hard to see if all the
operators are &&, etc.) and we avoid having to reformat as much if we add or
remove one.

[asgerf, 2015/03/06 10:01:37]

It's been bothering me too. I'll rename it to .value.
isDeclaration is true when the assignment has to be a declaration and thus can't
be hoisted. This is necessary to properly scope captured variables inside a
loop.

As mentioned in the CL comment, this could be loosened, but then
copyPropagateVariable has to return a variable AND a boolean indicating if the
resulting assignment should be a declaration. I just couldn't bother right now
because I think the copy propagator is going to be overhauled or removed when
the new registor allocator lands.

Come to think of it, since mutable variables are now also used for try
statements I've taken the opportunity to update the tree IR builder so
declarations are only added for mutable variables that are actually captured.
Done.

+        !node.variable.isCaptured && enclosingTrys == 0) {
       VariableUse definition = node.definition;
-      if (definition.variable.readCount == 1 &&
-          node.variable.host == currentElement) {
+      if (definition.variable.readCount == 1) {
         move[definition.variable] = node;
         inverseMove[node.variable] = node;
       }
     }
 
     return node;
   }
 
   Statement visitLabeledStatement(LabeledStatement node) {
     node.next = visitBasicBlock(node.next);
@@ skipping 24 matching lines @@
   Statement visitWhileTrue(WhileTrue node) {
     node.body = visitBasicBlock(node.body);
     return node;
   }
 
   Statement visitWhileCondition(WhileCondition node) {
     throw "WhileCondition before LoopRewriter";
   }
 
   Statement visitTry(Try node) {
+    enclosingTrys++;
     node.tryBody = visitBasicBlock(node.tryBody);
+    enclosingTrys--;
     node.catchBody = visitBasicBlock(node.catchBody);
     return node;
   }
 
   Statement visitFunctionDeclaration(FunctionDeclaration node) {
     // Unlike var declarations, function declarations are not hoisted, so we
     // can't do copy propagation of the variable.
     new CopyPropagator().rewrite(node.definition);
     node.next = visitStatement(node.next);
     return node;
@@ skipping 14 matching lines @@
 
   void visitFunctionExpression(FunctionExpression node) {
     new CopyPropagator().rewrite(node.definition);
   }
 
   void visitFieldInitializer(FieldInitializer node) {
     visitStatement(node.body);
   }
 
 }