dart2dart: Bugfix in copy propagator.

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

Index: pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart
diff --git a/pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart b/pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart
index 2f3761db4262d7f41dd1c2e282065160fbc19ad1..e9c82e0d068b63c6e853cc941d5d0a5b2b7f0aa8 100644
--- a/pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart
+++ b/pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart
@@ -23,6 +23,9 @@ class CopyPropagator extends RecursiveVisitor with PassMixin {
 
   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);
@@ -168,11 +171,12 @@ class CopyPropagator extends RecursiveVisitor with PassMixin {
 
     // 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;
       }
@@ -217,7 +221,9 @@ class CopyPropagator extends RecursiveVisitor with PassMixin {
   }
 
   Statement visitTry(Try node) {
+    enclosingTrys++;
     node.tryBody = visitBasicBlock(node.tryBody);
+    enclosingTrys--;
     node.catchBody = visitBasicBlock(node.catchBody);
     return node;
   }