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

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.
 
 library tree_ir.optimization.statement_rewriter;
 
 import 'optimization.dart' show Pass;
 import '../tree_ir_nodes.dart';
 import '../../io/source_information.dart';
 import '../../elements/elements.dart';
@@ skipping 149 matching lines @@
   /// Assignments with constant right-hand sides (see [isEffectivelyConstant])
   /// are not considered impure and are put in [constantEnvironment] instead.
   ///
   /// Except for [Conditional]s, expressions in the environment have
   /// not been processed, and all their subexpressions must therefore be
   /// variables uses.
   List<Expression> environment = <Expression>[];
 
   /// 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'.
   Map<Label, Jump> labelRedirects = <Label, Jump>{};
 
   /// 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>{};
 
   /// Number of assignments to a given variable that dominate the current
   /// position.
   ///
   /// Pure expressions will not be inlined if it uses a variable with more than
   /// one dominating assignment, because the reaching definition of the used
   /// 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
   /// 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>();
 
+  /// 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) {
     Jump newJump = labelRedirects[jump.target];
     return newJump != null ? newJump : jump;
   }
 
   void inEmptyEnvironment(void action(), {bool keepConstants: true}) {
     List oldEnvironment = environment;
     Map oldConstantEnvironment = constantEnvironment;
@@ skipping 105 matching lines @@
         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 (getRightHandVariable(binding) == node.variable) {
+      if (isAllowingRhsPropagation &&
+          getRightHandVariable(binding) == node.variable) {
         environment.removeLast();
         --node.variable.readCount;
         return visitExpression(binding);
       }
     }
 
     // If the definition could not be propagated, leave the variable use.
     return node;
   }
 
@@ skipping 134 matching lines @@
     //
     if (node.variable.readCount == 0) {
       --node.variable.writeCount;
       return node.value;
     }
     return node;
   }
 
   /// 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) {
     _rewriteList(node.arguments);
     return node;
   }
 
   Expression visitInvokeMethod(InvokeMethod node) {
     if (node.receiverIsNotNull) {
       _rewriteList(node.arguments);
@@ skipping 215 matching lines @@
       node.tryBody = visitStatement(node.tryBody);
       safeForInlining = saved;
       node.catchParameters.forEach(pushDominatingAssignment);
       node.catchBody = visitStatement(node.catchBody);
       node.catchParameters.forEach(popDominatingAssignment);
     });
     return node;
   }
 
   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());
       }
     }
     return node;
   }
 
   Expression visitThis(This node) {
     return node;
@@ skipping 138 matching lines @@
   ///    var y = bar();
   ///    var z = y < x;
   ///
   ///      ==>
   ///
   ///    var z = foo() > bar();
   ///
   /// 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;
   }
 
   /// If [s] and [t] are similar statements we extract their subexpressions
   /// and returns a new statement of the same type using expressions combined
   /// with the [combine] callback. For example:
   ///
   ///   combineStatements(Return E1, Return E2) = Return combine(E1, E2)
   ///
   /// If [combine] returns E1 then the unified statement is equivalent to [s],
@@ skipping 390 matching lines @@
   VariableUseCallback callback;
 
   VariableUseVisitor(this.callback);
 
   visitVariableUse(VariableUse use) => callback(use);
 
   static void visit(Expression node, VariableUseCallback callback) {
     new VariableUseVisitor(callback).visitExpression(node);
   }
 }