tree-ir bugfix: Count unseen uses instead of seen uses.

pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.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;
 
 /**
  * Performs the following transformations on the tree:
  * - Assignment inlining
  * - Assignment expression propagation
@@ skipping 119 matching lines @@
   List<Expression> environment = <Expression>[];
 
   /// Binding environment for variables that are assigned to effectively
   /// constant expressions (see [isEffectivelyConstant]).
   final Map<Variable, Expression> constantEnvironment;
 
   /// Substitution map for labels. Any break to a label L should be substituted
   /// for a break to L' if L maps to L'.
   Map<Label, Jump> labelRedirects = <Label, Jump>{};
 
-  /// Number of uses seen so far. Used to detect the first use of a variable
-  /// (since we do backwards traversal, the first use is the last one seen).
-  Map<Variable, int> seenUses = <Variable, int>{};
+  /// Number of uses of the given variable that are still unseen.
+  /// Used to detect the first use of a variable (since we do backwards
+  /// traversal, the first use is the last one seen).
+  Map<Variable, int> unseenUses = <Variable, int>{};
 
   /// Rewriter for methods.
   StatementRewriter({this.isDartMode})
       : constantEnvironment = <Variable, Expression>{} {
     assert(isDartMode != null);
   }
 
   /// Rewriter for nested functions.
   StatementRewriter.nested(StatementRewriter parent)
       : constantEnvironment = parent.constantEnvironment,
-        seenUses = parent.seenUses,
+        unseenUses = parent.unseenUses,
         isDartMode = parent.isDartMode;
 
   /// A set of labels that can be safely inlined at their use.
   ///
   /// The successor statements for labeled statements that have only one break
   /// from them are normally rewritten inline at the site of the break.  This
   /// is not safe if the code would be moved inside the scope of an exception
   /// handler (i.e., if the code would be moved into a try from outside it).
   Set<Label> safeForInlining = new Set<Label>();
 
@@ skipping 32 matching lines @@
   /// If the given expression always returns the value of one of its
   /// subexpressions, and that subexpression is a variable use, returns that
   /// variable. Otherwise `null`.
   Variable getRightHand(Expression e) {
     Expression value = getValueSubexpression(e);
     return value is VariableUse ? value.variable : null;
   }
 
   @override
   Expression visitVariableUse(VariableUse node) {
-    // Count of number of uses seen so far.
-    seenUses[node.variable] = 1 + seenUses.putIfAbsent(node.variable, () => 0);
+    // Count of number of unseen uses remaining.
+    unseenUses.putIfAbsent(node.variable, () => node.variable.readCount);
+    --unseenUses[node.variable];
 
     // We traverse the tree right-to-left, so when we have seen all uses,
     // it means we are looking at the first use.
-    assert(seenUses[node.variable] <= node.variable.readCount);
-    bool isFirstUse = seenUses[node.variable] == node.variable.readCount;
+    assert(unseenUses[node.variable] < node.variable.readCount);
+    assert(unseenUses[node.variable] >= 0);
+    bool isFirstUse = unseenUses[node.variable] == 0;
 
     // Propagate constant to use site.
     Expression constant = constantEnvironment[node.variable];
     if (constant != null) {
       --node.variable.readCount;
-      --seenUses[node.variable]; // Do not count the use we just destroyed.

[asgerf, 2015/04/16 12:03:55]

I knew this was a bad idea and somehow I did it anyway :(

       return visitExpression(constant);
     }
 
     // Try to propagate another expression into this variable use.
     if (!environment.isEmpty) {
       Expression binding = environment.last;
 
       // Is this variable assigned by the most recently evaluated impure
       // expression?
       //
       // If so, propagate the assignment, e.g:
       //
       //     { x = foo(); bar(x, x) } ==> bar(x = foo(), x)
       //
       // We must ensure that no other uses separate this use from the
       // assignment. We therefore only propagate assignments into the first use.
       //
       // Note that if this is only use, `visitAssign` will then remove the
       // redundant assignment.
       if (getLeftHand(binding) == node.variable && isFirstUse) {
         environment.removeLast();
         --node.variable.readCount;
-        --seenUses[node.variable]; // Do not count the use we just destroyed.
         return visitExpression(binding);
       }
 
       // Is the most recently evaluated impure expression known to have the
       // value of this variable?
       //
       // If so, we can replace this use with the impure expression, e.g:
       //
       //     { E.foo = x; bar(x) } ==> bar(E.foo = x)
       //
       if (getRightHand(binding) == node.variable) {
         environment.removeLast();
         --node.variable.readCount;
-        --seenUses[node.variable];
         return visitExpression(binding);
       }
     }
 
     // If the definition could not be propagated, leave the variable use.
     return node;
   }
 
   /// Returns true if [exp] has no side effects and has a constant value within
   /// any given activation of the enclosing method.
@@ skipping 627 matching lines @@
 }
 
 /// Combines two variable uses into one.
 class CombinedUses implements CombinedExpressions {
   VariableUse use1, use2;
   Expression combined;
 
   CombinedUses(this.use1, this.use2) {
     assert(use1.variable == use2.variable);
     use1.variable.readCount -= 2; // Destroy both the original uses.
     combined = new VariableUse(use1.variable);

[asgerf, 2015/04/16 12:03:55]

FYI this is where the bookkeeping was missing.

Updating seenUses here would work, but clearly the issue would come back to
haunt us again.

   }
 
   void uncombine() {
     ++use1.variable.readCount; // Restore original reference count.
   }
 }
 
 /// Result of combining two expressions that do not affect reference counting.
 class GenericCombinedExpressions implements CombinedExpressions {
   Expression combined;
 
   GenericCombinedExpressions(this.combined);
 
   void uncombine() {}
 }