Change the type inference for fields in dart2js

lib/compiler/implementation/ssa/optimize.dart

Index: lib/compiler/implementation/ssa/optimize.dart
diff --git a/lib/compiler/implementation/ssa/optimize.dart b/lib/compiler/implementation/ssa/optimize.dart
index b019498dbf3ad920d500bb87f646ccb58298080c..d4d828cac5a71cba4e42e37c7e7ad27d72349ae8 100644
--- a/lib/compiler/implementation/ssa/optimize.dart
+++ b/lib/compiler/implementation/ssa/optimize.dart
@@ -47,7 +47,7 @@ class SsaOptimizerTask extends CompilerTask {
           // opportunities for constant folding.
           new SsaConstantFolder(constantSystem, backend, work, types),
           new SsaDeadCodeEliminator(types),
-          new SsaRegisterRecompilationCandidates(backend, work, types)];
+          new SsaConstructionFieldTypes(backend, work, types)];
       runPhases(graph, phases);
     });
   }
@@ -62,7 +62,6 @@ class SsaOptimizerTask extends CompilerTask {
     return measure(() {
       // Run the phases that will generate type guards.
       List<OptimizationPhase> phases = <OptimizationPhase>[
-          new SsaRecompilationFieldTypePropagator(backend, work, types),
           new SsaSpeculativeTypePropagator(compiler, types),
           new SsaTypeGuardInserter(compiler, work, types),
           new SsaEnvironmentBuilder(compiler),
@@ -612,23 +611,17 @@ class SsaConstantFolder extends HBaseVisitor implements OptimizationPhase {
       // initializer list.
       isFinalOrConst = true;
     }
-    if (!isFinalOrConst &&
-        !compiler.codegenWorld.hasInvokedSetter(field, compiler) &&
-        !compiler.codegenWorld.hasFieldSetter(field, compiler)) {
-      switch (compiler.phase) {
-        case Compiler.PHASE_COMPILING:
-          compiler.enqueuer.codegen.registerRecompilationCandidate(
-              work.element);
-          break;
-        case Compiler.PHASE_RECOMPILING:
-          // If field is not final or const but no setters are used then the
-          // field might be considered final anyway as it will be either
-          // un-initialized or initialized in the constructor initializer list.
-          isFinalOrConst = true;
-          break;
+    HFieldGet result = new HFieldGet(
+        field, node.inputs[0], isAssignable: !isFinalOrConst);
+    if (work.element.isMember()) {
+      HType type = backend.optimisticFieldType(field);
+      if (type != null) {
+        result.guaranteedType = type;
+        backend.registerFieldTypesOptimization(
+            work.element, field, result.guaranteedType);
       }
     }
-    return new HFieldGet(field, node.inputs[0], isAssignable: !isFinalOrConst);
+    return result;
   }
 
   HInstruction visitInvokeDynamicSetter(HInvokeDynamicSetter node) {
@@ -1220,159 +1213,156 @@ class SsaTypeConversionInserter extends HBaseVisitor
 }
 
 
-// Base class for the handling of recompilation based on inferred
-// field types.
-class BaseRecompilationVisitor extends HBaseVisitor {
+// Analyze the constructors to see if some fields will always have a specific
+// type after construction. This is mainly done to override to possible null

[ngeoffray, 2012/09/20 14:43:01]

to override to -> to override.

But the sentence is still hard to parse.

[Søren Gjesse, 2012/09/21 13:49:44]

Rephrased.

+// value set by field initializer.
+class SsaConstructionFieldTypes
+    extends HBaseVisitor implements OptimizationPhase {
   final JavaScriptBackend backend;
   final WorkItem work;
   final HTypeMap types;
-  Compiler get compiler => backend.compiler;
+  final String name = "SsaConstructionFieldTypes";
+  final Set<Element> allSetters;
+  final Map<Element, HType> exitDominatedSetters;
+  final Map<HBasicBlock, Map<Element, HType>> blockFieldSetters;
+  bool thisExposed = false;
+  HGraph currentGraph;
+  bool collectSetters;
+  Map<Element, HType> currentFieldSetters;
+
+  SsaConstructionFieldTypes(JavaScriptBackend this.backend,
+         WorkItem this.work,
+         HTypeMap this.types)
+      : allSetters = new Set<Element>(),
+        exitDominatedSetters = new Map<Element, HType>(),
+        blockFieldSetters = new Map<HBasicBlock, Map<Element, HType>>();
 
-  BaseRecompilationVisitor(this.backend, this.work, this.types);
-
-  abstract void handleFieldGet(HFieldGet node, HType type);
-  abstract void handleFieldNumberOperation(HFieldGet field, HType type);
-
-  // Checks if the binary invocation operates on a field and a
-  // constant number. If it does [handleFieldNumberOperation] is
-  // called with the field and the type inferred for the field so far.
-  void checkFieldNumberOperation(HInvokeBinary node) {
-    // Determine if one of the operands is an HFieldGet.
-    HFieldGet field;
-    HInstruction other;
-    if (node.left is HFieldGet) {
-      field = node.left;
-      other = node.right;
-    } else if (node.right is HFieldGet) {
-      field = node.right;
-      other = node.left;
-    }
-    // Try to optimize the case where a field which is known to always
-    // be an integer is compared with a constant number.
-    if (other != null &&
-        other.isConstantNumber() &&
-        field.element != null &&
-        field.element.isMember()) {
-      // Calculate the field type from the information available.  If
-      // we have type information for the field and it contains NUMBER
-      // we use it as a candidate for recompilation.
-      Element fieldElement = field.element;
-      HType fieldSettersType = backend.fieldSettersTypeSoFar(fieldElement);
-      HType initializersType = backend.typeFromInitializersSoFar(fieldElement);
-      HType fieldType = fieldSettersType.union(initializersType);
-      HType type = HType.NUMBER.union(fieldType);
-      if (type == HType.NUMBER) {
-        handleFieldNumberOperation(field, fieldType);
-      }
-    }
+  void visitGraph(HGraph graph) {
+    currentGraph = graph;
+    if (!work.element.isGenerativeConstructorBody() &&
+        !work.element.isGenerativeConstructor()) return;
+    visitDominatorTree(graph);
   }
 
-  void visitFieldGet(HFieldGet node) {
-    if (!node.element.isInstanceMember()) return;
-    Element field = node.element;
-    if (field != null) {
-      HType type = backend.optimisticFieldTypeAfterConstruction(field);
-      if (!type.isUnknown()) {
-        // Allow handling even if we haven't seen any types for this
-        // field yet. There might still be only one setter in an
-        // initializer list or constructor body and recompilation
-        // can therefore pay off.
-        handleFieldGet(node, type);
+  visitBasicBlock(HBasicBlock node) {
+    currentBlock = node;

[ngeoffray, 2012/09/20 14:43:01]

Do you need currentBlock?

[Søren Gjesse, 2012/09/21 13:49:44]

No. It is an instance variable in the HBaseVisitor base class though. Removed
the line.

+    collectSetters = true;
+    currentFieldSetters = null;
+    if (node.predecessors.length == 1) {
+      // If there is just one predeecessor start out with the field setters
+      // from there.
+      currentFieldSetters = blockFieldSetters[node.predecessors[0]];
+    } else {
+      // If there is more than one predeecessor combine the field setters from
+      // all of them.

[ngeoffray, 2012/09/20 14:43:01]

I don't really like these null checks here. Could you have the map of a block be
a copy the map of its first predecessor (or a new Map for the first block), and
then intersect it with the maps of the previous predecessors?

[Søren Gjesse, 2012/09/21 13:49:44]

Done.

+      node.predecessors.forEach((HBasicBlock block) {
+        Map<Element, HType> predecessorsFieldSetters = blockFieldSetters[block];
+        if (predecessorsFieldSetters === null) {
+          currentFieldSetters = null;
+          collectSetters = false;
+        } else if (collectSetters) {
+          if (currentFieldSetters === null) {
+            // Just copy the predecessor setters if this is the first
+            // predecessor.
+            currentFieldSetters = predecessorsFieldSetters;
+          } else {
+            Map<Element, HType> newFieldSetters = new Map<Element, HType>();
+            currentFieldSetters.forEach((Element element, HType currentType) {
+              HType type = predecessorsFieldSetters[element];
+              if (type != null) {
+                newFieldSetters[element] = currentType.union(type);
+              }
+            });
+            currentFieldSetters = newFieldSetters;
+          }
+        }
+      });
+    }
+    HInstruction instruction = node.first;

[ngeoffray, 2012/09/20 14:43:01]

Change this loop to block.forEachInstruction(..._

[Søren Gjesse, 2012/09/21 13:49:44]

Done.

+    while (instruction !== null) {
+      instruction.accept(this);
+      instruction = instruction.next;
+    }
+    if (currentFieldSetters != null) {
+      blockFieldSetters[node] = currentFieldSetters;
+      if (node.dominates(currentGraph.exit)) {
+        currentFieldSetters.forEach((Element field, HType type) {
+          exitDominatedSetters[field] = type;

[ngeoffray, 2012/09/20 14:43:01]

Do you need this? I would think that when you reach the exit block, the
intersection of its predecessors would contain the list of fields that have been
initialized.

[Søren Gjesse, 2012/09/21 13:49:44]

The exitDominatedSetters is now gone.

+        });
       }
     }
   }
 
-  HInstruction visitEquals(HEquals node) {
-    checkFieldNumberOperation(node);
+  visitInstruction(HInstruction instruction) {
+    // Instruction not explicitly handled stops the collection of field setter
+    // information.
+    currentFieldSetters = null;
+    collectSetters = false;
   }
 
-  HInstruction visitBinaryArithmetic(HBinaryArithmetic node) {
-    checkFieldNumberOperation(node);
+  visitForeignNew(HForeignNew node) {

[ngeoffray, 2012/09/20 14:43:01]

Document which element has a foreign new, and what it means to go through its
arguments.

[Søren Gjesse, 2012/09/21 13:49:44]

Done.

+    int j = 0;
+    node.element.forEachInstanceField(
+      includeBackendMembers: true,
+      includeSuperMembers: true,
+      f: (ClassElement enclosingClass, Element element) {
+        backend.registerFieldInitializer(element, types[node.inputs[j]]);
+        j++;
+      });
   }
-}
-
-
-// Visitor that registers candidates for recompilation.
-class SsaRegisterRecompilationCandidates
-    extends BaseRecompilationVisitor implements OptimizationPhase {
-  final String name = "SsaRegisterRecompileCandidates";
-  HGraph graph;
 
-  SsaRegisterRecompilationCandidates(JavaScriptBackend backend,
-                                     WorkItem work,
-                                     HTypeMap types)
-      : super(backend, work, types);
+  visitStatic(HStatic node) {
+    // Ignore static.
+  }
 
-  void visitGraph(HGraph visitee) {
-    graph = visitee;
-    if (compiler.phase == Compiler.PHASE_COMPILING) {
-      visitDominatorTree(visitee);
+  visitInvoke(HInvoke node) {
+    for (int i = 0; i < node.inputs.length && !thisExposed; i++) {
+      if (node.inputs[i] is HThis) thisExposed = true;
     }
   }
 
-  void handleFieldGet(HFieldGet node, HType type) {
-    assert(compiler.phase == Compiler.PHASE_COMPILING);
-    compiler.enqueuer.codegen.registerRecompilationCandidate(
-        work.element);
+  visitFieldSet(HFieldSet node) {
+    Element field = node.element;
+    HInstruction value = node.value;
+    HType type = types[value];
+    allSetters.add(field);
+    if (work.element.isGenerativeConstructorBody() &&

[ngeoffray, 2012/09/20 14:43:01]

Why this first check? Otherwise it's a generative constructor and it does not
have a body.

[Søren Gjesse, 2012/09/21 13:49:44]

Removed. I was not 100% sure of what the generative constructor could contain.

+        node.element.isMember() &&

[ngeoffray, 2012/09/20 14:43:01]

Shouldn't  you also check that the class of the element is the current class?

[Søren Gjesse, 2012/09/21 13:49:44]

Done.

+        node.value.hasGuaranteedType() &&
+        node.block.dominates(currentGraph.exit)) {

[ngeoffray, 2012/09/20 14:43:01]

Again, I don't think this is needed if you just look at the fieldSetters of the
exit block.

[Søren Gjesse, 2012/09/21 13:49:44]

Done.

+      exitDominatedSetters[field] = value.guaranteedType;
+    } else {

[ngeoffray, 2012/09/20 14:43:01]

I don't understand why an else here.

[Søren Gjesse, 2012/09/21 13:49:44]

The code here is now simplified.

+      if (collectSetters) {
+        if (type.isUseful()) {
+          if (currentFieldSetters === null) {
+            currentFieldSetters = new Map<Element, HType>();
+          }
+          if (currentFieldSetters.containsKey(field)) {
+            currentFieldSetters[field] = currentFieldSetters[field].union(type);

[ngeoffray, 2012/09/20 14:43:01]

Maybe fetch currentFieldSetters[field] before, and check for null.

[Søren Gjesse, 2012/09/21 13:49:44]

Done.

+          } else {
+            currentFieldSetters[field] = type;
+          }
+        }
+      }
+    }
   }
 
-  void handleFieldNumberOperation(HFieldGet node, HType type) {
-    assert(compiler.phase == Compiler.PHASE_COMPILING);
-    compiler.enqueuer.codegen.registerRecompilationCandidate(
-        work.element);
+  visitGoto(HGoto node) {
+    // Ignore gotos.
   }
-}
-
-
-// Visitor that sets the known or suspected type of fields during
-// recompilation.
-class SsaRecompilationFieldTypePropagator
-    extends BaseRecompilationVisitor implements OptimizationPhase {
-  final String name = "SsaRecompilationFieldTypePropagator";
-  HGraph graph;
-
-  SsaRecompilationFieldTypePropagator(JavaScriptBackend backend,
-                                      WorkItem work,
-                                      HTypeMap types)
-      : super(backend, work, types);
 
-  void visitGraph(HGraph visitee) {
-    graph = visitee;
-    if (compiler.phase == Compiler.PHASE_RECOMPILING) {
-      visitDominatorTree(visitee);
-    }
-  }
-
-  void handleFieldGet(HFieldGet field, HType type) {
-    assert(compiler.phase == Compiler.PHASE_RECOMPILING);
-    if (!type.isConflicting()) {
-      // If there are no invoked setters with this name, the union of
-      // the types of the initializers and the setters is guaranteed
-      // otherwise it is only speculative.
-      Element element = field.element;
-      assert(!element.isGenerativeConstructorBody());
-      if (!compiler.codegenWorld.hasInvokedSetter(element, compiler)) {
-        field.guaranteedType =
-            type.union(backend.fieldSettersTypeSoFar(element));
-      } else {
-        types[field] = type.union(backend.fieldSettersTypeSoFar(element));
-      }
-    }
-  }
+  visitExit(HExit node) {
+    // If this has been exposed then we cannot say anything about types after
+    // construction.
+    if (thisExposed) return;
 
-  void handleFieldNumberOperation(HFieldGet field, HType type) {
-    assert(compiler.phase == Compiler.PHASE_RECOMPILING);
-    if (compiler.codegenWorld.hasInvokedSetter(field.element, compiler)) {
-      // If there are invoked setters we don't know for sure
-      // that the field will hold a value of the calculated
-      // type, but the fact that the class itself sticks to
-      // this type for the field is still a strong signal
-      // indicating the expected type of the field.
-      types[field] = type;
-    } else {
-      // If there are no invoked setters we know the type of
-      // this field for sure.
-      field.guaranteedType = type;
-    }
+    // Register the known field types.
+    exitDominatedSetters.forEach((Element element, HType type) {
+      backend.registerFieldConstructor(element, type);
+      allSetters.remove(element);
+    });
+    allSetters.forEach((Element element) {
+      backend.registerFieldConstructor(element, HType.UNKNOWN);
+    });
   }
 }