Validate that all instructions dominate their inputs. And fix a bug where that did not happen.

lib/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.
 
 class Interceptors {
   Compiler compiler;
   Interceptors(Compiler this.compiler);
 
   SourceString mapOperatorToMethodName(Operator op) {
     String name = op.source.stringValue;
@@ skipping 419 matching lines @@
         ClassElement cls = element.enclosingElement.enclosingElement;
         Type type = cls.computeType(builder.compiler);
         cachedTypeOfThis = new HBoundedType.nonNull(type);
       }
       res.guaranteedType = cachedTypeOfThis;
     }
     return res;
   }
 
   HParameterValue getActivationParameter(Element element) {
-    if (element.isParameter()) {
-      HInstruction instruction = directLocals[element];
-      if (instruction is HParameterValue) return instruction;
-    }
+    // If the element is a parameter, we already have a

[floitsch, 2012/05/30 11:52:38]

Can you also add a comment, why we have this 'if'?
Unless I'm wrong this is an optimization to avoid redundant assignments.
Correct?

[ngeoffray, 2012/05/30 12:37:26]

There is a comment:
If the element is a parameter, we already have a HParameterValue for it.

It's just to avoid adding unnecessary new HParameterValue. So I'm not sure what
you think is missing from the comment?

[floitsch, 2012/05/30 12:59:41]

If I'm not wrong this is just to avoid wrapping parameters. Removing this line
would not change anything.
So what I would like to see (as comment): is this necessary, or just an
optimization.

[ngeoffray, 2012/05/31 08:18:33]

It is necessary because we need to share the same name between the
HParameterValue (real parameter) and the HFieldGet and HFieldSet instructions.
I'll add that as a comment.

+    // HParameterValue for it.
+    if (element.isParameter()) return directLocals[element];
 
     return builder.activationVariables.putIfAbsent(element, () {
       HParameterValue parameter = new HParameterValue(element);
       builder.graph.entry.addAtExit(parameter);
       return parameter;
     });
   }
 
   /**
    * Sets the [element] to [value]. If the element is boxed or stored in a
@@ skipping 84 matching lines @@
   }
 
   void beginLoopHeader(Node node, HBasicBlock loopEntry) {
     // Create a copy because we modify the map while iterating over
     // it.
     Map<Element, HInstruction> saved =
         new Map<Element, HInstruction>.from(directLocals);
 
     // Create phis for all elements in the definitions environment.
     saved.forEach((Element element, HInstruction instruction) {
-      // We know 'this' cannot be modified.
-      if (element !== closureData.thisElement) {
-        HPhi phi = new HPhi.singleInput(element, instruction);
-        loopEntry.addPhi(phi);
-        directLocals[element] = phi;
-      } else {
-        directLocals[element] = instruction;
+      if (isAccessedDirectly(element)) {
+        // We know 'this' cannot be modified.
+        if (element !== closureData.thisElement) {
+          HPhi phi = new HPhi.singleInput(element, instruction);
+          loopEntry.addPhi(phi);
+          directLocals[element] = phi;
+        } else {
+          directLocals[element] = instruction;
+        }
       }
     });
   }
 
   void enterLoopBody(Node node) {
     ClosureScope scopeData = closureData.capturingScopes[node];
     if (scopeData == null) return;
     // If there are no declared boxed loop variables then we did not create the
     // box before the initializer and we have to create the box now.
     if (!scopeData.hasBoxedLoopVariables()) {
@@ skipping 2546 matching lines @@
   visitSwitchCase(SwitchCase node) {
     compiler.internalError('SsaBuilder.visitSwitchCase');
   }
 
   visitCaseMatch(CaseMatch node) {
     compiler.internalError('SsaBuilder.visitCaseMatch');
   }
 
   visitTryStatement(TryStatement node) {
     work.allowSpeculativeOptimization = false;
+    // Save the current locals. The catch block and the finally block
+    // must not reuse the existing locals handler. None of the variables
+    // that have been defined in the body-block will be used, but for
+    // loops we will add (unnecessary) phis that will reference the body
+    // variables. This will make it look as if the variables were used

[Lasse Reichstein Nielsen, 2012/05/30 09:35:14]

"will make" -> "makes" (prefer active voice where possible)

[ngeoffray, 2012/05/30 10:19:07]

Done.

+    // in a non-dominated block.
+    LocalsHandler savedLocals = new LocalsHandler.from(localsHandler);
     HBasicBlock enterBlock = openNewBlock();
     HTry tryInstruction = new HTry();
     List<HBasicBlock> blocks = <HBasicBlock>[];
     blocks.add(close(tryInstruction));
 
     HBasicBlock tryBody = graph.addNewBlock();
     enterBlock.addSuccessor(tryBody);
     open(tryBody);
     visit(node.tryBlock);
     if (!isAborted()) blocks.add(close(new HGoto()));
     SubGraph bodyGraph = new SubGraph(tryBody, lastOpenedBlock);
     SubGraph catchGraph = null;
     HParameterValue exception = null;
     if (!node.catchBlocks.isEmpty()) {
+      localsHandler = new LocalsHandler.from(savedLocals);
       HBasicBlock block = graph.addNewBlock();
       enterBlock.addSuccessor(block);
       open(block);
       // Note that the name of this element is irrelevant.
       Element element = new Element(
           const SourceString('exception'), ElementKind.PARAMETER, work.element);
       exception = new HParameterValue(element);
       add(exception);
       HInstruction oldRethrowableException = rethrowableException;
       rethrowableException = exception;
@@ skipping 46 matching lines @@
           handleIf(() { pushCondition(newBlock); },
                    visitThen, visitElse);
         }
       }
 
       CatchBlock firstBlock = link.head;
       handleIf(() { pushCondition(firstBlock); }, visitThen, visitElse);
       if (!isAborted()) blocks.add(close(new HGoto()));
 
       rethrowableException = oldRethrowableException;
+      tryInstruction.catchBlock = block;
       catchGraph = new SubGraph(block, lastOpenedBlock);
     }
 
     SubGraph finallyGraph = null;
     if (node.finallyBlock != null) {
+      localsHandler = new LocalsHandler.from(savedLocals);
       HBasicBlock finallyBlock = graph.addNewBlock();
       enterBlock.addSuccessor(finallyBlock);
       open(finallyBlock);
       visit(node.finallyBlock);
       if (!isAborted()) blocks.add(close(new HGoto()));
       tryInstruction.finallyBlock = finallyBlock;
       finallyGraph = new SubGraph(finallyBlock, lastOpenedBlock);
     }
 
     HBasicBlock exitBlock = graph.addNewBlock();
 
     for (HBasicBlock block in blocks) {
       block.addSuccessor(exitBlock);
     }
 
+    // Use the locals handler not altered by the catch and finally
+    // blocks.
+    localsHandler = savedLocals;
     open(exitBlock);
     enterBlock.setBlockFlow(
         new HTryBlockInformation(
           wrapStatementGraph(bodyGraph),
           exception,
           wrapStatementGraph(catchGraph),
           wrapStatementGraph(finallyGraph)),
         exitBlock);
   }
 
@@ skipping 158 matching lines @@
                                    <HInstruction>[target, input],
                                    HType.STRING));
     return builder.pop();
   }
 
   HInstruction result(Node node) {
     flushLiterals(node);
     return prefix;
   }
 }