dart2js: Allow 'throw' when inlining

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

 // Copyright (c) 2012, 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 ssa;
 
 /**
  * A special element for the extra parameter taken by intercepted
  * methods. We need to override [Element.computeType] because our
  * optimizers may look at its declared type.
@@ skipping 18 matching lines @@
       super(backend.compiler);
 
   HGraph build(CodegenWorkItem work) {
     return measure(() {
       Element element = work.element.implementation;
       HInstruction.idCounter = 0;
       ConstantSystem constantSystem = compiler.backend.constantSystem;
       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,
                                         'unexpected element kind $kind');
       }
       assert(graph.isValid());
       if (!identical(kind, ElementKind.FIELD)) {
         FunctionElement function = element;
         graph.calledInLoop = compiler.world.isCalledInLoop(function);
         OptionalParameterTypes defaultValueTypes = null;
@@ skipping 46 matching lines @@
   }
 
   HGraph compileConstructor(SsaBuilder builder, CodegenWorkItem work) {
     // The body of the constructor will be generated in a separate function.
     final ClassElement classElement = work.element.getEnclosingClass();
     return builder.buildFactory(classElement.implementation,
                                 work.element.implementation);
   }
 }
 
+
 /**
  * Keeps track of locals (including parameters and phis) when building. The
  * 'this' reference is treated as parameter and hence handled by this class,
  * too.
  */
 class LocalsHandler {
   /**
    * The values of locals that can be directly accessed (without redirections
    * to boxes or closure-fields).
    *
@@ skipping 310 matching lines @@
       HInstruction box = readLocal(redirect.enclosingElement);
       builder.add(new HFieldSet(redirect, box, value));
     } else {
       assert(isUsedInTry(element));
       HLocalValue local = getLocal(element);
       builder.add(new HLocalSet(element, local, value));
     }
   }
 
   /**
-   * 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
    * [For] loops) at the correct places. For phi-handling [beginLoopHeader] and
    * [endLoop] must also be called.
    *
    * The correct place for the box depends on the given loop. In most cases
    * the box will be created when entering the loop-body: while, do-while, and
    * for-in (assuming the call to [:next:] is inside the body) can always be
    * constructed this way.
@@ skipping 332 matching lines @@
   /**
    * Variables stored in the current activation. These variables are
    * being updated in try/catch blocks, and should be
    * accessed indirectly through [HLocalGet] and [HLocalSet].
    */
   Map<Element, HLocalValue> activationVariables;
 
   // 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;
   Compiler get compiler => builder.compiler;
   CodeEmitterTask get emitter => builder.emitter;
 
   SsaBuilder(this.constantSystem, SsaBuilderTask builder, CodegenWorkItem work)
     : this.builder = builder,
       this.backend = builder.backend,
       this.work = work,
       graph = new HGraph(),
       stack = new List<HInstruction>(),
       activationVariables = new Map<Element, HLocalValue>(),
       jumpTargets = new Map<TargetElement, JumpHandler>(),
       parameters = new Map<Element, HInstruction>(),
       sourceElementStack = <Element>[work.element],
       inliningStack = <InliningState>[],
       rti = builder.backend.rti,
       super(work.resolutionTree) {
     localsHandler = new LocalsHandler(this);
   }
 
   static const MAX_INLINING_DEPTH = 3;
   static const MAX_INLINING_NODES = 46;
   List<InliningState> inliningStack;
   Element returnElement;
   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
    * internal error.
    */
   Constant compileConstant(VariableElement element) {
     return compiler.constantHandler.compileConstant(element);
   }
 
   Constant compileVariable(VariableElement element) {
@@ skipping 903 matching lines @@
 
   void visit(Node node) {
     if (node != null) node.accept(this);
   }
 
   visitBlock(Block node) {
     for (Link<Node> link = node.statements.nodes;
          !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;
       }
     }
     assert(!current.isClosed());
     if (!stack.isEmpty) compiler.cancel('non-empty instruction stack');
   }
 
   visitClassNode(ClassNode node) {
     compiler.internalError('visitClassNode should not be called', node: node);
   }
 
   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();
+    }
   }
 
   /**
    * Creates a new loop-header block. The previous [current] block
    * is closed with an [HGoto] and replaced by the newly created block.
    * Also notifies the locals handler that we're entering a loop.
    */
   JumpHandler beginLoopHeader(Node node) {
     assert(!isAborted());
     HBasicBlock previousBlock = close(new HGoto());
@@ skipping 256 matching lines @@
           HBasicBlock block = breakInstruction.block;
           block.addAtExit(new HBreak.toLabel(label));
           block.remove(breakInstruction);
         });
       }
     }
     jumpHandler.close();
   }
 
   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;
       Node initializer = node.initializer;
       if (initializer != null) {
         visit(initializer);
         if (initializer.asExpression() != null) {
           pop();
         }
       }
@@ skipping 14 matching lines @@
         HInstruction updateInstruction = pop();
       }
     }
     void buildBody() {
       visit(node.body);
     }
     handleLoop(node, buildInitializer, buildCondition, buildUpdate, buildBody);
   }
 
   visitWhile(While node) {
+    if (!isReachable) return;
     HInstruction buildCondition() {
       visit(node.condition);
       return popBoolified();
     }
     handleLoop(node,
                () {},
                buildCondition,
                () {},
                () { visit(node.body); });
   }
 
   visitDoWhile(DoWhile node) {
+    if (!isReachable) return;
     LocalsHandler savedLocals = new LocalsHandler.from(localsHandler);
     localsHandler.startLoop(node);
     JumpHandler jumpHandler = beginLoopHeader(node);
     HLoopInformation loopInfo = current.loopInformation;
     HBasicBlock loopEntryBlock = current;
     HBasicBlock bodyEntryBlock = current;
     TargetElement target = elements[node];
     bool hasContinues = target != null && target.isContinueTarget;
     if (hasContinues) {
       // Add extra block to hang labels on.
@@ skipping 142 matching lines @@
       }
     });
 
     HType type = new HType.nonNullExact(
         compiler.functionClass.computeType(compiler),
         compiler);
     push(new HForeignNew(closureClassElement, type, capturedVariables));
   }
 
   visitFunctionDeclaration(FunctionDeclaration node) {
+    if (!isReachable) return;
     visit(node.function);
     localsHandler.updateLocal(elements[node], pop());
   }
 
   visitIdentifier(Identifier node) {
     if (node.isThis()) {
       stack.add(localsHandler.readThis());
     } else {
       compiler.internalError("SsaBuilder.visitIdentifier on non-this",
                              node: node);
     }
   }
 
   visitIf(If node) {
+    if (!isReachable) return;
     handleIf(node,
              () => visit(node.condition),
              () => visit(node.thenPart),
              node.elsePart != null ? () => visit(node.elsePart) : null);
   }
 
   void handleIf(Node diagnosticNode,
                 void visitCondition(), void visitThen(), void visitElse()) {
     SsaBranchBuilder branchBuilder = new SsaBranchBuilder(this, diagnosticNode);
     branchBuilder.handleIf(visitCondition, visitThen, visitElse);
@@ skipping 1585 matching lines @@
     open(newBlock);
   }
 
   visitReturn(Return node) {
     if (identical(node.getBeginToken().stringValue, 'native')) {
       native.handleSsaNative(this, node.expression);
       return;
     }
     assert(invariant(node, !node.isRedirectingFactoryBody));
     HInstruction value;
-    if (node.expression == null) {
+    if (node.expression == null || !isReachable) {
       value = graph.addConstantNull(constantSystem);
     } else {
       visit(node.expression);
       value = pop();
       value = potentiallyCheckType(value, returnType);
     }
 
     handleInTryStatement();
 
     if (!inliningStack.isEmpty) {
       localsHandler.updateLocal(returnElement, value);
     } else {
       close(attachPosition(new HReturn(value), node)).addSuccessor(graph.exit);
     }
   }
 
   visitThrow(Throw node) {
     if (node.expression == null) {
       HInstruction exception = rethrowableException;
       if (exception == null) {
         exception = graph.addConstantNull(constantSystem);
         compiler.internalError(
             'rethrowableException should not be null', node: node);
       }
       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;
+      }
     }
   }
 
   visitTypeAnnotation(TypeAnnotation node) {
     compiler.internalError('visiting type annotation in SSA builder',
                            node: node);
   }
 
   visitVariableDefinitions(VariableDefinitions node) {
+    if (!isReachable) return;
     for (Link<Node> link = node.definitions.nodes;
          !link.isEmpty;
          link = link.tail) {
       Node definition = link.head;
       if (definition is Identifier) {
         HInstruction initialValue = graph.addConstantNull(constantSystem);
         localsHandler.updateLocal(elements[definition], initialValue);
       } else {
         assert(definition is SendSet);
         visitSendSet(definition);
@@ skipping 973 matching lines @@
     }
     node.visitChildren(this);
     seenReturn = true;
   }
 
   void visitTryStatement(Node node) {
     if (!registerNode()) return;
     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;
   }
 }
 
 class InliningState {
   /**
    * Documentation wanted -- johnniwinther
    *
    * Invariant: [function] must be an implementation element.
    */
   final PartialFunctionElement function;
@@ skipping 241 matching lines @@
           new HSubGraphBlockInformation(elseBranch.graph));
 
     HBasicBlock conditionStartBlock = conditionBranch.block;
     conditionStartBlock.setBlockFlow(info, joinBlock);
     SubGraph conditionGraph = conditionBranch.graph;
     HIf branch = conditionGraph.end.last;
     assert(branch is HIf);
     branch.blockInformation = conditionStartBlock.blockFlow;
   }
 }