dart2js cps: Disable RHS propagation in operands to builtin operators.

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

Index: pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart
diff --git a/pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart b/pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart
index 32b508ff2cc92ed7615804b77b4ba6fc98597151..abfc9cb2f73b077b63ea0c7ecaf88331efb75119 100644
--- a/pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart
+++ b/pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart
@@ -167,7 +167,7 @@ class StatementRewriter extends Transformer implements Pass {
 
   /// Binding environment for variables that are assigned to effectively
   /// constant expressions (see [isEffectivelyConstant]).
-  Map<Variable, Expression> constantEnvironment;
+  Map<Variable, Expression> constantEnvironment = <Variable, Expression>{};
 
   /// Substitution map for labels. Any break to a label L should be substituted
   /// for a break to L' if L maps to L'.
@@ -186,14 +186,6 @@ class StatementRewriter extends Transformer implements Pass {
   /// variable might have changed since it was put in the environment.
   final Map<Variable, int> dominatingAssignments = <Variable, int>{};
 
-  /// Rewriter for methods.
-  StatementRewriter() : constantEnvironment = <Variable, Expression>{};
-
-  /// Rewriter for nested functions.
-  StatementRewriter.nested(StatementRewriter parent)
-      : constantEnvironment = parent.constantEnvironment,
-        unseenUses = parent.unseenUses;
-

[asgerf, 2016/01/06 00:18:41]

Drive-by cleanup

   /// A set of labels that can be safely inlined at their use.
   ///
   /// The successor statements for labeled statements that have only one break
@@ -202,6 +194,13 @@ class StatementRewriter extends Transformer implements Pass {
   /// handler (i.e., if the code would be moved into a try from outside it).
   Set<Label> safeForInlining = new Set<Label>();
 
+  /// If the top element is true, assignments of form "x = CONST" may be
+  /// propagated into a following occurence of CONST.  This may confuse the JS
+  /// runtime so it is disabled in some cases.

[sra1, 2016/01/06 00:41:06]

'engine' rather than 'runtime'.

[asgerf, 2016/01/06 15:43:31]

Done.

+  List<bool> allowRhsPropagation = <bool>[true];

[sra1, 2016/01/06 00:41:06]

I assume it was too nasty to thread through as an argument.

[asgerf, 2016/01/06 15:43:31]

Yeah.

+
+  bool get isAllowingRhsPropagation => allowRhsPropagation.last;

[sra1, 2016/01/06 00:41:06]

Maybe rename to isRhsPropagationAllowed.
('are you allowing ...' vs 'is it allowed ...')

[asgerf, 2016/01/06 15:43:31]

Done.

+
   /// Returns the redirect target of [jump] or [jump] itself if it should not
   /// be redirected.
   Jump redirect(Jump jump) {
@@ -327,7 +326,8 @@ class StatementRewriter extends Transformer implements Pass {
       //
       //     { E.foo = x; bar(x) } ==> bar(E.foo = x)
       //
-      if (getRightHandVariable(binding) == node.variable) {
+      if (isAllowingRhsPropagation &&
+          getRightHandVariable(binding) == node.variable) {
         environment.removeLast();
         --node.variable.readCount;
         return visitExpression(binding);
@@ -482,10 +482,12 @@ class StatementRewriter extends Transformer implements Pass {
 
   /// Process nodes right-to-left, the opposite of evaluation order in the case
   /// of argument lists..
-  void _rewriteList(List<Node> nodes) {
+  void _rewriteList(List<Node> nodes, {bool rhsPropagation: true}) {
+    allowRhsPropagation.add(rhsPropagation);
     for (int i = nodes.length - 1; i >= 0; --i) {
       nodes[i] = visitExpression(nodes[i]);
     }
+    allowRhsPropagation.removeLast();
   }
 
   Expression visitInvokeStatic(InvokeStatic node) {
@@ -721,7 +723,7 @@ class StatementRewriter extends Transformer implements Pass {
   }
 
   Expression visitConstant(Constant node) {
-    if (!environment.isEmpty) {
+    if (isAllowingRhsPropagation && !environment.isEmpty) {
       Constant constant = getRightHandConstant(environment.last);
       if (constant != null && constant.value == node.value) {
         return visitExpression(environment.removeLast());
@@ -880,31 +882,28 @@ class StatementRewriter extends Transformer implements Pass {
   /// foo() must be evaluated before bar(), so the propagation is only possible
   /// by commuting the operator.
   Expression visitApplyBuiltinOperator(ApplyBuiltinOperator node) {
-    if (environment.isEmpty || getLeftHand(environment.last) == null) {
-      // If there is no recent assignment that might propagate, so there is no
-      // opportunity for optimization here.
-      _rewriteList(node.arguments);
-      return node;
-    }
-    Variable propagatableVariable = getLeftHand(environment.last);
-    BuiltinOperator commuted = commuteBinaryOperator(node.operator);
-    if (commuted != null) {
-      assert(node.arguments.length == 2); // Only binary operators can commute.
-      Expression left = node.arguments[0];
-      if (left is VariableUse && propagatableVariable == left.variable) {
-        Expression right = node.arguments[1];
-        if (right is This ||
-            (right is VariableUse &&
-             propagatableVariable != right.variable &&
-             !constantEnvironment.containsKey(right.variable))) {
-          // An assignment can be propagated if we commute the operator.
-          node.operator = commuted;
-          node.arguments[0] = right;
-          node.arguments[1] = left;
+    if (!environment.isEmpty && getLeftHand(environment.last) != null) {
+      Variable propagatableVariable = getLeftHand(environment.last);
+      BuiltinOperator commuted = commuteBinaryOperator(node.operator);
+      if (commuted != null) {
+        // Only binary operators can commute.
+        assert(node.arguments.length == 2);
+        Expression left = node.arguments[0];
+        if (left is VariableUse && propagatableVariable == left.variable) {
+          Expression right = node.arguments[1];
+          if (right is This ||
+              (right is VariableUse &&
+               propagatableVariable != right.variable &&
+               !constantEnvironment.containsKey(right.variable))) {
+            // An assignment can be propagated if we commute the operator.
+            node.operator = commuted;
+            node.arguments[0] = right;
+            node.arguments[1] = left;
+          }
         }
       }
     }
-    _rewriteList(node.arguments);
+    _rewriteList(node.arguments, rhsPropagation: false);

[sra1, 2016/01/06 00:41:06]

Add a comment why rhsPropagation is false.

    if (p == (q.f = null)) ...

is slow on V8.

The problem is only with V8, only when the operation is == (!=), and only when
one operand is `null`.
V8 generates efficient code for `x == null` only when `null` is syntactically an
operand, otherwise it is very slow.

[asgerf, 2016/01/06 15:43:31]

Done.

     return node;
   }