dart2js cps: Refactor CallExpressions into Primitives.

pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart

Index: pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart
diff --git a/pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart b/pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart
index 50e84bdfec263e44e83d33db2ab1904f3cc69187..d3e59e6032fee97b0ba39500fe2e4cb67acb33ef 100644
--- a/pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart
+++ b/pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart
@@ -123,12 +123,6 @@ abstract class InteriorExpression extends Expression implements InteriorNode {
   }
 }
 
-/// An expression that passes a continuation to a call.
-abstract class CallExpression extends Expression {
-  Reference<Continuation> get continuation;
-  Expression get next => continuation.definition.body;
-}
-
 /// An expression without a continuation or a subexpression body.
 ///
 /// These break straight-line control flow and can be thought of as ending a
@@ -268,6 +262,33 @@ abstract class Primitive extends Variable<Primitive> {
     assert(hasNoUses);
     RemovalVisitor.remove(this);
   }
+
+  /// Replaces this definition, both at the binding site and at all uses sites.

[sra1, 2015/11/19 21:41:46]

Then maybe call it 'replaceWith'.

[asgerf, 2015/11/20 16:23:54]

I would like that name, but 'substituteFor' and 'replaceWith' sound too much
like they're doing the same thing (albeit with arguments reversed).

I went ahead and did the following renames:
  x.substituteFor(y) --> y.replaceUsesWith(x)
  x.redefineAs(y) --> x.replaceWith(y)

I think 'replaceUsesWith' and 'replaceWith' have a nice consistency, and it's
clear how they are different.

I did this in a separate patch, please take a look at it and see if the client
code is more/less readable than before.

+  ///
+  /// This can be thought of as changing the definition of a `let` while
+  /// preserving the variable name:
+  ///
+  ///     let x = OLD in BODY
+  ///       ==>
+  ///     let x = NEW in BODY
+  ///
+  void redefineAs(Primitive newDefinition) {

[sra1, 2015/11/19 21:41:45]

assert(newDefinition.parent == null);
assert there are no uses.

[asgerf, 2015/11/20 16:23:53]

There are uses. They are being replaced with uses of 'newDefinition' in the
first line, and then destroy() asserts that there are no more uses.

+    newDefinition.substituteFor(this);
+    destroy();
+    LetPrim let = parent;
+    let.primitive = newDefinition;
+    newDefinition.parent = let;
+    newDefinition.useElementAsHint(hint);
+  }
+}
+
+/// A primitive that is generally not safe for elimination, but may be marked
+/// as safe by type propagation
+//
+// TODO(asgerf): Store the flag in a bitmask in [Primitive] and get rid of this
+//               class.
+abstract class UnsafePrimitive extends Primitive {
+  bool isSafeForElimination = false;
 }
 
 /// Operands to invocations and primitives are always variables.  They point to
@@ -435,35 +456,49 @@ class LetMutable extends InteriorExpression {
 /// Discussion:
 /// All information in the [selector] is technically redundant; it will likely
 /// be removed.
-class InvokeStatic extends CallExpression {
+class InvokeStatic extends UnsafePrimitive {
   final FunctionElement target;
   final Selector selector;
   final List<Reference<Primitive>> arguments;
-  final Reference<Continuation> continuation;
   final SourceInformation sourceInformation;
 
+  bool get isSafeForReordering => isSafeForElimination;
+
   InvokeStatic(this.target,
                this.selector,
                List<Primitive> args,
-               Continuation cont,
                [this.sourceInformation])
-      : arguments = _referenceList(args),
-        continuation = new Reference<Continuation>(cont);
+      : arguments = _referenceList(args);
 
   InvokeStatic.byReference(this.target,
                            this.selector,
                            this.arguments,
-                           this.continuation,
                            [this.sourceInformation]);
 
   accept(Visitor visitor) => visitor.visitInvokeStatic(this);
 
   void setParentPointers() {
     _setParentsOnList(arguments, this);
-    continuation.parent = this;
   }
 }
 
+enum CallingConvention {

[sra1, 2015/11/19 21:41:45]

Just a thought:

We might want to make this a hand-rolled enum so the calling convention
constants can have properties.

Rather than convention==Normal, we would ask !convention.hasInterceptorPosition

[asgerf, 2015/11/20 16:23:54]

I don't know that "interceptor position" means, but I get the idea. Let's not
overdo it, though, I think we should wait and see if we need it

FWIW, I'm planning to introduce a bitmask for all primitives, holding things
such as isSafeForElimination as well as the calling convention.
The calling convention will probably be split up into two bits: "where is the
receiver" and "are the argument shifted or not" (with the invalid combination
being unused),
corresponding to the two predicates we could put on the enum objects.

+  /// JS receiver is the Dart receiver, there are no extra arguments.
+  ///
+  /// For example: `foo.bar$1(x)`
+  Normal,

[sra1, 2015/11/19 21:41:45]

Maybe 'Direct'. 'Normal' implies abnormal but interceptors are well formed and
widespread.

[asgerf, 2015/11/20 16:23:54]

I would also consider NonIntercepted to be "direct" (in the sense that it calls
directly on the Dart receiver), it just has a dummy argument.

It's often hard to name the "baseline" thing, because everything else is defined
in terms of how they are different from this baseline.
So it has no defining characteristics except not being special in any way.

Normal, Plain, Ordinary, Common, Simple, Standard, pick your favourite.

+
+  /// JS receiver is an interceptor, the first argument is the Dart receiver.
+  ///
+  /// For example: `getInterceptor(foo).bar$1(foo, x)`
+  Intercepted,
+
+  /// JS receiver is the Dart receiver, the first argument is a dummy value.
+  ///
+  /// For example: `foo.bar$1(0, x)`
+  DummyIntercepted,
+}
+
 /// Invoke a method on an object.
 ///
 /// This includes getters, setters, operators, and index getter/setters.
@@ -473,43 +508,52 @@ class InvokeStatic extends CallExpression {
 ///
 /// The [selector] records the names of named arguments. The value of named
 /// arguments occur at the end of the [arguments] list, in normalized order.
-class InvokeMethod extends CallExpression {
+class InvokeMethod extends UnsafePrimitive {
   Reference<Primitive> receiver;
   Selector selector;
   TypeMask mask;
   final List<Reference<Primitive>> arguments;
-  final Reference<Continuation> continuation;
   final SourceInformation sourceInformation;
 
-  /// If true, the [receiver] is intercepted and the actual receiver is in
-  /// the first argument. Otherwise, the [receiver] is the actual receiver.
-  ///
-  /// This flag is always false for non-intercepted selectors, but it may also
-  /// be false for intercepted selectors after dummy receiver optimization
-  /// (in this case the first argument is a dummy value).
-  ///
-  /// It is always false before the unsugaring pass, where interceptors have
-  /// not yet been introduced.
-  bool receiverIsIntercepted = false;
+  CallingConvention callingConvention = CallingConvention.Normal;
+
+  bool get receiverIsIntercepted {

[sra1, 2015/11/19 21:41:45]

I'm wondering if there are any inconsistencies between receiverIsIntercepted and
checking the calling convention.
Perhaps we should delete this member.

[asgerf, 2015/11/20 16:23:54]

Done.

+    return callingConvention == CallingConvention.Intercepted;
+  }
+
+  Reference<Primitive> get dartReceiverRef {

[sra1, 2015/11/19 21:41:46]

We try to use full words where possible, e.g. dartReceiverReference

[asgerf, 2015/11/20 16:23:54]

Done.

+    return callingConvention == CallingConvention.Intercepted
+        ? arguments[0]
+        : receiver;
+  }
+
+  Primitive get dartReceiver => dartReceiverRef.definition;
+
+  Reference<Primitive> getDartArgumentRef(int n) {

[sra1, 2015/11/19 21:41:45]

can we drop the 'get' on these methods for symmetry with the receiver?

[asgerf, 2015/11/20 16:23:53]

Done.

+    return callingConvention == CallingConvention.Normal
+        ? arguments[n]
+        : arguments[n + 1];
+  }
+
+  Primitive getDartArgument(int n) => getDartArgumentRef(n).definition;
 
   /// If true, it is known that the receiver cannot be `null`.
   bool receiverIsNotNull = false;
 
+  bool get isSafeForReordering => isSafeForElimination;

[sra1, 2015/11/19 21:41:46]

This feels backwards.

I'd rather this was a separate flag on UnsafePrimitive.

A function that is a static field getter is always safe for elimination but not
ordering.

[asgerf, 2015/11/20 16:23:53]

Done.

+
   InvokeMethod(Primitive receiver,
                this.selector,
                this.mask,
                List<Primitive> arguments,
-               Continuation continuation,
                [this.sourceInformation])
       : this.receiver = new Reference<Primitive>(receiver),
-        this.arguments = _referenceList(arguments),
-        this.continuation = new Reference<Continuation>(continuation);
+        this.arguments = _referenceList(arguments);
 
   InvokeMethod.byReference(this.receiver,
                            this.selector,
                            this.mask,
                            this.arguments,
-                           this.continuation,
                            this.sourceInformation);
 
   accept(Visitor visitor) => visitor.visitInvokeMethod(this);
@@ -517,7 +561,6 @@ class InvokeMethod extends CallExpression {
   void setParentPointers() {
     receiver.parent = this;
     _setParentsOnList(arguments, this);
-    continuation.parent = this;
   }
 }
 
@@ -540,30 +583,50 @@ class InvokeMethod extends CallExpression {
 ///
 /// All optional arguments declared by [target] are passed in explicitly, and
 /// occur at the end of [arguments] list, in normalized order.
-class InvokeMethodDirectly extends CallExpression {
+class InvokeMethodDirectly extends UnsafePrimitive {
   Reference<Primitive> receiver;
   final FunctionElement target;
   final Selector selector;
   final List<Reference<Primitive>> arguments;
-  final Reference<Continuation> continuation;
   final SourceInformation sourceInformation;
 
+  CallingConvention callingConvention = CallingConvention.Normal;
+
+  bool get receiverIsIntercepted {
+    return callingConvention == CallingConvention.Intercepted;
+  }
+
+  Reference<Primitive> get dartReceiverRef {
+    return callingConvention == CallingConvention.Intercepted
+        ? arguments[0]
+        : receiver;
+  }
+
+  Primitive get dartReceiver => dartReceiverRef.definition;
+
+  Reference<Primitive> getDartArgumentRef(int n) {
+    return callingConvention == CallingConvention.Normal
+        ? arguments[n]
+        : arguments[n + 1];
+  }
+
+  Primitive getDartArgument(int n) => getDartArgumentRef(n).definition;
+
+  bool get isSafeForReordering => isSafeForElimination;
+
   InvokeMethodDirectly(Primitive receiver,
                        this.target,
                        this.selector,
                        List<Primitive> arguments,
-                       Continuation continuation,
                        this.sourceInformation)
       : this.receiver = new Reference<Primitive>(receiver),
-        this.arguments = _referenceList(arguments),
-        this.continuation = new Reference<Continuation>(continuation);
+        this.arguments = _referenceList(arguments);
 
   accept(Visitor visitor) => visitor.visitInvokeMethodDirectly(this);
 
   void setParentPointers() {
     receiver.parent = this;
     _setParentsOnList(arguments, this);
-    continuation.parent = this;
   }
 }
 
@@ -582,14 +645,15 @@ class InvokeMethodDirectly extends CallExpression {
 ///
 /// Note that [InvokeConstructor] does it itself allocate an object.
 /// The invoked constructor will do that using [CreateInstance].
-class InvokeConstructor extends CallExpression {
+class InvokeConstructor extends UnsafePrimitive {
   final DartType dartType;
   final ConstructorElement target;
   final List<Reference<Primitive>> arguments;
-  final Reference<Continuation> continuation;
   final Selector selector;
   final SourceInformation sourceInformation;
 
+  bool get isSafeForReordering => isSafeForElimination;
+
   /// If non-null, this is an allocation site-specific type that is potentially
   /// better than the inferred return type of [target].
   ///
@@ -601,17 +665,14 @@ class InvokeConstructor extends CallExpression {
                     this.target,
                     this.selector,
                     List<Primitive> args,
-                    Continuation cont,
                     this.sourceInformation,
                     {this.allocationSiteType})
-      : arguments = _referenceList(args),
-        continuation = new Reference<Continuation>(cont);
+      : arguments = _referenceList(args);
 
   accept(Visitor visitor) => visitor.visitInvokeConstructor(this);
 
   void setParentPointers() {
     _setParentsOnList(arguments, this);
-    continuation.parent = this;
   }
 }
 
@@ -719,26 +780,26 @@ class TypeTestViaFlag extends Primitive {
 /// [value], which is typically in scope in the continuation. However, it might
 /// simplify type propagation, since a better type can be computed for the
 /// continuation parameter without needing flow-sensitive analysis.
-class TypeCast extends CallExpression {
+class TypeCast extends UnsafePrimitive {
   Reference<Primitive> value;
   final DartType dartType;
 
   /// See the corresponding field on [TypeTest].
   final List<Reference<Primitive>> typeArguments;
-  final Reference<Continuation> continuation;
+
+  bool get isSafeForReordering => isSafeForElimination;
 
   TypeCast(Primitive value,
            this.dartType,
-           List<Primitive> typeArguments,
-           Continuation cont)
+           List<Primitive> typeArguments)
       : this.value = new Reference<Primitive>(value),
-        this.typeArguments = _referenceList(typeArguments),
-        this.continuation = new Reference<Continuation>(cont);
+        this.typeArguments = _referenceList(typeArguments);
 
   accept(Visitor visitor) => visitor.visitTypeCast(this);
 
   void setParentPointers() {
     value.parent = this;
+    _setParentsOnList(typeArguments, this);
   }
 }
 
@@ -1126,22 +1187,17 @@ class SetStatic extends Primitive {
 ///
 /// If the field has not yet been initialized, its initializer is evaluated
 /// and assigned to the field.
-///
-/// [continuation] is then invoked with the value of the field as argument.
-class GetLazyStatic extends CallExpression {
+class GetLazyStatic extends UnsafePrimitive {
   final FieldElement element;
-  final Reference<Continuation> continuation;
   final SourceInformation sourceInformation;
 
-  GetLazyStatic(this.element,
-                Continuation continuation,
-                [this.sourceInformation])
-      : continuation = new Reference<Continuation>(continuation);
+  bool get isSafeForReordering => isSafeForElimination;
+
+  GetLazyStatic(this.element, [this.sourceInformation]);
 
   accept(Visitor visitor) => visitor.visitGetLazyStatic(this);
 
   void setParentPointers() {
-    continuation.parent = this;
   }
 }
 
@@ -1303,24 +1359,23 @@ class CreateInvocationMirror extends Primitive {
   }
 }
 
-class ForeignCode extends CallExpression {
+class ForeignCode extends UnsafePrimitive {
   final js.Template codeTemplate;
   final TypeMask type;
   final List<Reference<Primitive>> arguments;
   final native.NativeBehavior nativeBehavior;
   final FunctionElement dependency;
-  final Reference<Continuation> continuation;
+
+  bool get isSafeForReordering => isSafeForElimination;
 
   ForeignCode(this.codeTemplate, this.type, List<Primitive> arguments,
-      this.nativeBehavior, Continuation continuation, {this.dependency})
-      : this.arguments = _referenceList(arguments),
-        this.continuation = new Reference<Continuation>(continuation);
+      this.nativeBehavior, {this.dependency})
+      : this.arguments = _referenceList(arguments);
 
   accept(Visitor visitor) => visitor.visitForeignCode(this);
 
   void setParentPointers() {
     _setParentsOnList(arguments, this);
-    continuation.parent = this;
   }
 }
 
@@ -1576,13 +1631,13 @@ class TypeExpression extends Primitive {
   }
 }
 
-class Await extends CallExpression {
+class Await extends UnsafePrimitive {
   final Reference<Primitive> input;
-  final Reference<Continuation> continuation;
 
-  Await(Primitive input, Continuation continuation)
-    : this.input = new Reference<Primitive>(input),
-      this.continuation = new Reference<Continuation>(continuation);
+  bool get isSafeForReordering => isSafeForElimination;
+
+  Await(Primitive input)
+    : this.input = new Reference<Primitive>(input);
 
   @override
   accept(Visitor visitor) {
@@ -1591,18 +1646,17 @@ class Await extends CallExpression {
 
   void setParentPointers() {
     input.parent = this;
-    continuation.parent = this;
   }
 }
 
-class Yield extends CallExpression {
+class Yield extends UnsafePrimitive {
   final Reference<Primitive> input;
-  final Reference<Continuation> continuation;
   final bool hasStar;
 
-  Yield(Primitive input, this.hasStar, Continuation continuation)
-    : this.input = new Reference<Primitive>(input),
-      this.continuation = new Reference<Continuation>(continuation);
+  bool get isSafeForReordering => isSafeForElimination;
+
+  Yield(Primitive input, this.hasStar)
+    : this.input = new Reference<Primitive>(input);
 
   @override
   accept(Visitor visitor) {
@@ -1611,7 +1665,6 @@ class Yield extends CallExpression {
 
   void setParentPointers() {
     input.parent = this;
-    continuation.parent = this;
   }
 }
 
@@ -1760,7 +1813,6 @@ class DeepRecursiveVisitor implements Visitor {
   processInvokeStatic(InvokeStatic node) {}
   visitInvokeStatic(InvokeStatic node) {
     processInvokeStatic(node);
-    processReference(node.continuation);
     node.arguments.forEach(processReference);
   }
 
@@ -1775,7 +1827,6 @@ class DeepRecursiveVisitor implements Visitor {
   visitInvokeMethod(InvokeMethod node) {
     processInvokeMethod(node);
     processReference(node.receiver);
-    processReference(node.continuation);
     node.arguments.forEach(processReference);
   }
 
@@ -1783,14 +1834,12 @@ class DeepRecursiveVisitor implements Visitor {
   visitInvokeMethodDirectly(InvokeMethodDirectly node) {
     processInvokeMethodDirectly(node);
     processReference(node.receiver);
-    processReference(node.continuation);
     node.arguments.forEach(processReference);
   }
 
   processInvokeConstructor(InvokeConstructor node) {}
   visitInvokeConstructor(InvokeConstructor node) {
     processInvokeConstructor(node);
-    processReference(node.continuation);
     node.arguments.forEach(processReference);
   }
 
@@ -1816,7 +1865,6 @@ class DeepRecursiveVisitor implements Visitor {
   processTypeCast(TypeCast node) {}
   visitTypeCast(TypeCast node) {
     processTypeCast(node);
-    processReference(node.continuation);
     processReference(node.value);
     node.typeArguments.forEach(processReference);
   }
@@ -1845,7 +1893,6 @@ class DeepRecursiveVisitor implements Visitor {
   processGetLazyStatic(GetLazyStatic node) {}
   visitGetLazyStatic(GetLazyStatic node) {
     processGetLazyStatic(node);
-    processReference(node.continuation);
   }
 
   processLiteralList(LiteralList node) {}
@@ -1972,9 +2019,6 @@ class DeepRecursiveVisitor implements Visitor {
   processForeignCode(ForeignCode node) {}
   visitForeignCode(ForeignCode node) {
     processForeignCode(node);
-    if (node.continuation != null) {
-      processReference(node.continuation);
-    }
     node.arguments.forEach(processReference);
   }
 
@@ -1987,14 +2031,12 @@ class DeepRecursiveVisitor implements Visitor {
   visitAwait(Await node) {
     processAwait(node);
     processReference(node.input);
-    processReference(node.continuation);
   }
 
   processYield(Yield node) {}
   visitYield(Yield node) {
     processYield(node);
     processReference(node.input);
-    processReference(node.continuation);
   }
 
   processGetLength(GetLength node) {}