From c85916dedc773135bdb4bb4a79112b2ab1a18b52 Mon Sep 17 00:00:00 2001
From: Asger Feldthaus <asgerf@google.com>
Date: Tue, 12 Jan 2016 15:18:31 -0800
Subject: [PATCH] dart2js cps: Hoist unsafe expressions from loop entry.

Primitives that potentially throw cannot normally be hoisted, but we can
sometimes hoist them anyway if they are at the loop entry.

We now hoist GetLength, GetField, GetIndex, NullCheck, and BoundsCheck.

BUG=
R=sigmund@google.com

Review URL: https://codereview.chromium.org/1578963002 .
---
 pkg/compiler/lib/src/cps_ir/gvn.dart          | 50 ++++++++++++++++---
 .../dart2js/cps_ir/expected/codeUnitAt_2.js   |  3 +-
 2 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/pkg/compiler/lib/src/cps_ir/gvn.dart b/pkg/compiler/lib/src/cps_ir/gvn.dart
index c22db8b35364..a4a591f3ae28 100644
--- a/pkg/compiler/lib/src/cps_ir/gvn.dart
+++ b/pkg/compiler/lib/src/cps_ir/gvn.dart
@@ -176,15 +176,48 @@ class GVN extends TrampolineRecursiveVisitor implements Pass {
     return next;
   }
 
+  bool isFirstImpureExpressionInLoop(Expression exp) {
+    InteriorNode node = exp.parent;
+    for (; node is Expression; node = node.parent) {
+      if (node is LetPrim && node.primitive.isSafeForElimination) {
+        continue;
+      }
+      if (node is LetCont) {
+        continue;
+      }
+      return false;
+    }
+    return node == currentLoopHeader;
+  }
+
+  bool isHoistablePrimitive(Primitive prim) {
+    if (prim.isSafeForElimination) return true;
+    if (prim is NullCheck ||
+        prim is BoundsCheck ||
+        prim is GetLength ||
+        prim is GetField ||
+        prim is GetIndex) {
+      // Expressions that potentially throw but have no other effects can be
+      // hoisted if they occur as the first impure expression in a loop.
+      // Note regarding BoundsCheck: the current array length is an input to
+      // check, so the check itself has no heap dependency.  It will only be
+      // hoisted if the length was hoisted.
+      // TODO(asgerf): In general we could hoist these out of multiple loops,
+      //   but the trick we use here only works for one loop level.
+      return isFirstImpureExpressionInLoop(prim.parent);
+    }
+    return false;
+  }
+
   /// Try to hoist the binding of [prim] out of loops. Returns `true` if it was
   /// hoisted or marked as a trivial hoist-on-demand primitive.
   bool tryToHoistOutOfLoop(Primitive prim, int gvn) {
-    // Do not hoist primitives with side effects.
-    if (!prim.isSafeForElimination) return false;
-
     // Bail out fast if the primitive is not inside a loop.
     if (currentLoopHeader == null) return false;
 
+    // Do not hoist primitives with side effects.
+    if (!isHoistablePrimitive(prim)) return false;
+
     LetPrim letPrim = prim.parent;
 
     // Find the depth of the outermost scope where we can bind the primitive
@@ -327,9 +360,15 @@ class GVN extends TrampolineRecursiveVisitor implements Pass {
   }
 
   /// Assuming [prim] has no side effects, returns true if it can safely
-  /// be hoisted out of [loop] without changing its value.
+  /// be hoisted out of [loop] without changing its value or changing the timing
+  /// of a thrown exception.
   bool canHoistHeapDependencyOutOfLoop(Primitive prim, Continuation loop) {
-    assert(prim.isSafeForElimination);
+    // If the primitive might throw, we have to check that it is the first
+    // impure expression in the loop.  This has already been checked if
+    // [loop] is the current loop header, but for other loops we just give up.
+    if (!prim.isSafeForElimination && loop != currentLoopHeader) {
+      return false;
+    }
     if (prim is GetLength && !isImmutableLength(prim)) {
       return !loopEffects.loopChangesLength(loop);
     } else if (prim is GetField && !isImmutable(prim.field)) {
@@ -343,7 +382,6 @@ class GVN extends TrampolineRecursiveVisitor implements Pass {
     }
   }
 
-
   // ------------------ TRAVERSAL AND EFFECT NUMBERING ---------------------
   //
   // These methods traverse the IR while updating the current effect numbers.
diff --git a/tests/compiler/dart2js/cps_ir/expected/codeUnitAt_2.js b/tests/compiler/dart2js/cps_ir/expected/codeUnitAt_2.js
index 58429f15741e..73e278b67792 100644
--- a/tests/compiler/dart2js/cps_ir/expected/codeUnitAt_2.js
+++ b/tests/compiler/dart2js/cps_ir/expected/codeUnitAt_2.js
@@ -15,8 +15,9 @@ function() {
   for (; i < v0; sum += "ABC".charCodeAt(i), ++i)
     ;
   P.print(sum);
+  v0 = "Hello".length;
   sum = 0;
-  for (i = 0; i < "Hello".length; sum += "Hello".charCodeAt(i), ++i)
+  for (i = 0; i < v0; sum += "Hello".charCodeAt(i), ++i)
     ;
   P.print(sum);
 }