dart2js: Allow 'throw' when inlining

sdk/lib/_internal/compiler/implementation/ssa/builder.dart

Index: sdk/lib/_internal/compiler/implementation/ssa/builder.dart
diff --git a/sdk/lib/_internal/compiler/implementation/ssa/builder.dart b/sdk/lib/_internal/compiler/implementation/ssa/builder.dart
index 48dd75987ec7b1cb9ee4089227b4d814277a7a11..6bf1c4db380de893471221033a74aaa289181dc7 100644
--- a/sdk/lib/_internal/compiler/implementation/ssa/builder.dart
+++ b/sdk/lib/_internal/compiler/implementation/ssa/builder.dart
@@ -36,14 +36,14 @@ class SsaBuilderTask extends CompilerTask {
       SsaBuilder builder = new SsaBuilder(constantSystem, this, work);
       HGraph graph;
       ElementKind kind = element.kind;
-      if (identical(kind, ElementKind.GENERATIVE_CONSTRUCTOR)) {
+      if (kind == ElementKind.GENERATIVE_CONSTRUCTOR) {
         graph = compileConstructor(builder, work);
-      } else if (identical(kind, ElementKind.GENERATIVE_CONSTRUCTOR_BODY) ||
-                 identical(kind, ElementKind.FUNCTION) ||
-                 identical(kind, ElementKind.GETTER) ||
-                 identical(kind, ElementKind.SETTER)) {
+      } else if (kind == ElementKind.GENERATIVE_CONSTRUCTOR_BODY ||
+                 kind == ElementKind.FUNCTION ||
+                 kind == ElementKind.GETTER ||
+                 kind == ElementKind.SETTER) {
         graph = builder.buildMethod(element);
-      } else if (identical(kind, ElementKind.FIELD)) {
+      } else if (kind == ElementKind.FIELD) {
         graph = builder.buildLazyInitializer(element);
       } else {
         compiler.internalErrorOnElement(element,
@@ -110,6 +110,7 @@ class SsaBuilderTask extends CompilerTask {
   }
 }
 
+
 /**
  * Keeps track of locals (including parameters and phis) when building. The
  * 'this' reference is treated as parameter and hence handled by this class,
@@ -440,8 +441,9 @@ class LocalsHandler {
   }
 
   /**
-   * This function must be called before visiting any children of the loop. In
-   * particular it needs to be called before executing the initializers.
+   * This function, startLoop, must be called before visiting any children of
+   * the loop. In particular it needs to be called before executing the
+   * initializers.
    *
    * The [LocalsHandler] will make the boxes and updates at the right moment.
    * The builder just needs to call [enterLoopBody] and [enterLoopUpdates] (for
@@ -794,14 +796,27 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
   // We build the Ssa graph by simulating a stack machine.
   List<HInstruction> stack;
 
-  // The current block to add instructions to. Might be null, if we are
-  // visiting dead code.
-  HBasicBlock current;
-  // The most recently opened block. Has the same value as [current] while
-  // the block is open, but unlike [current], it isn't cleared when the current
-  // block is closed.
+  /**
+   * The current block to add instructions to. Might be null, if we are
+   * visiting dead code, but see [isReachable].
+   */
+  HBasicBlock _current;
+
+  /**
+   * The most recently opened block. Has the same value as [_current] while
+   * the block is open, but unlike [_current], it isn't cleared when the
+   * current block is closed.
+   */
   HBasicBlock lastOpenedBlock;
 
+  /**
+   * Indicates that the current block is dead (because it has a throw or a
+   * return further up.  If this is true, then [_current] may be null.  If it

[ngeoffray, 2013/04/10 12:06:39]

Missing ending paren after 'further up'.

[erikcorry, 2013/04/10 12:52:32]

Done.

+   * is dead then it may also be aborted, but for simplicity we only abort on
+   * statement boundaries, not in the middle of expressions.  See isAborted.
+   */
+  bool isReachable = true;

[karlklose, 2013/04/09 11:58:46]

The comment does not really match the name of the field.

[erikcorry, 2013/04/10 12:52:32]

Done.

+
   final List<Element> sourceElementStack;
 
   Element get currentElement => sourceElementStack.last.declaration;
@@ -831,6 +846,12 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
   DartType returnType;
   bool inTryStatement = false;
 
+  HBasicBlock get current => _current;
+  void set current(c) {
+    isReachable = c != null;
+    _current = c;
+  }
+
   /**
    * Compiles compile-time constants. Never returns [:null:]. If the
    * initial value is not a compile-time constants, it reports an
@@ -1754,7 +1775,7 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
          !link.isEmpty;
          link = link.tail) {
       visit(link.head);
-      if (isAborted()) {
+      if (!isReachable) {
         // The block has been aborted by a return or a throw.
         if (!stack.isEmpty) compiler.cancel('non-empty instruction stack');
         return;
@@ -1769,8 +1790,10 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
   }
 
   visitExpressionStatement(ExpressionStatement node) {
-    visit(node.expression);
-    pop();
+    if (isReachable) {

[ngeoffray, 2013/04/10 12:06:39]

It looks weird to check this here while the code line 1778 makes you think it
can never happen.

[erikcorry, 2013/04/10 12:52:32]

I replaced all these checks with asserts and it turns out that there is at least
one place in our tests where they are hit, so I changed it back.

[ngeoffray, 2013/04/10 13:02:58]

Isn't that a bug if that assert is hit? Do you know what test(s) failed?

[erikcorry, 2013/04/11 09:09:35]

It's the inlined throw test that I just introduced.  It's a ternary expression
where one of the 3 values threw, and one of the other branches is an inlined
function.  When we enter the visitBlock from the inlined method the isReachable
flag is already false before the first statement visited, and we don't test the
flag until after the first statement is visited.

I fixed visitBlock to test the flag before the first statement is visited.  The
net effect is the same as having these if's, which have now been replaced with
asserts.

+      visit(node.expression);
+      pop();
+    }
   }
 
   /**
@@ -2047,6 +2070,7 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
   }
 
   visitFor(For node) {
+    if (!isReachable) return;

[karlklose, 2013/04/09 11:58:46]

Would it be enough to abort in visitBlock instead of testing it for each
statement type?

[erikcorry, 2013/04/10 12:52:32]

I tried replacing this if with an assert and it was hit.  It seems harmless to
leave it as it is.

     assert(node.body != null);
     void buildInitializer() {
       if (node.initializer == null) return;
@@ -2081,6 +2105,7 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
   }
 
   visitWhile(While node) {
+    if (!isReachable) return;
     HInstruction buildCondition() {
       visit(node.condition);
       return popBoolified();
@@ -2093,6 +2118,7 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
   }
 
   visitDoWhile(DoWhile node) {
+    if (!isReachable) return;
     LocalsHandler savedLocals = new LocalsHandler.from(localsHandler);
     localsHandler.startLoop(node);
     JumpHandler jumpHandler = beginLoopHeader(node);
@@ -2255,6 +2281,7 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
   }
 
   visitFunctionDeclaration(FunctionDeclaration node) {
+    if (!isReachable) return;
     visit(node.function);
     localsHandler.updateLocal(elements[node], pop());
   }
@@ -2269,6 +2296,7 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
   }
 
   visitIf(If node) {
+    if (!isReachable) return;
     handleIf(node,
              () => visit(node.condition),
              () => visit(node.thenPart),
@@ -3874,7 +3902,7 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
     }
     assert(invariant(node, !node.isRedirectingFactoryBody));
     HInstruction value;
-    if (node.expression == null) {
+    if (node.expression == null || !isReachable) {
       value = graph.addConstantNull(constantSystem);
     } else {
       visit(node.expression);
@@ -3901,8 +3929,21 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
       }
       close(new HThrow(exception, isRethrow: true));
     } else {
-      visit(node.expression);
-      close(new HThrow(pop()));
+      if (inliningStack.isEmpty) {
+        if (isReachable) {

[ngeoffray, 2013/04/10 12:06:39]

I can't see why this can happen.

[erikcorry, 2013/04/10 12:52:32]

Perhaps if there is more than one throw in an inlined method?

[ngeoffray, 2013/04/10 13:02:58]

But the first throw would abort visiting the block.

[erikcorry, 2013/04/11 09:09:35]

I couldn't make this trigger so I removed it.

+          visit(node.expression);
+        } else {
+          stack.add(graph.addConstantNull(constantSystem));
+        }
+        close(new HThrow(pop()));
+      } else if (isReachable) {
+        // We don't close the block when we are inlining, because we could be
+        // inside an expression, and it is rather complicated to close the
+        // block at an arbitrary place in an expression.
+        visit(node.expression);
+        add(new HThrowExpression(pop()));
+        isReachable = false;
+      }
     }
   }
 
@@ -3912,6 +3953,7 @@ class SsaBuilder extends ResolvedVisitor implements Visitor {
   }
 
   visitVariableDefinitions(VariableDefinitions node) {
+    if (!isReachable) return;
     for (Link<Node> link = node.definitions.nodes;
          !link.isEmpty;
          link = link.tail) {
@@ -4905,9 +4947,11 @@ class InlineWeeder extends Visitor {
     tooDifficult = true;
   }
 
-  void visitThrow(Node node) {
+  void visitThrow(Throw node) {
     if (!registerNode()) return;
-    tooDifficult = true;
+    // We can't inline rethrows and we don't want to handle throw after a return
+    // even if it is in an "if".
+    if (seenReturn || node.expression == null) tooDifficult = true;
   }
 }