cps_ir: interceptor calling convention, operator== prologue

pkg/compiler/lib/src/js_backend/codegen/unsugar.dart

Index: pkg/compiler/lib/src/js_backend/codegen/unsugar.dart
diff --git a/pkg/compiler/lib/src/js_backend/codegen/unsugar.dart b/pkg/compiler/lib/src/js_backend/codegen/unsugar.dart
index a071b47a3b16a5fe29305fb9f578dfc0c252f040..555c5324257b1946d6910550e91540037b8765c7 100644
--- a/pkg/compiler/lib/src/js_backend/codegen/unsugar.dart
+++ b/pkg/compiler/lib/src/js_backend/codegen/unsugar.dart
@@ -6,23 +6,77 @@ import '../../cps_ir/cps_ir_nodes.dart';
 import '../../cps_ir/optimizers.dart';
 import '../../constants/expressions.dart';
 import '../../constants/values.dart';
-import '../../elements/elements.dart' show ClassElement, FieldElement, Element;
+import '../../elements/elements.dart' show
+    ClassElement,
+    ExecutableElement,
+    FieldElement,
+    Local;
 import '../../js_backend/codegen/glue.dart';
 import '../../dart2jslib.dart' show Selector, World;
 
+
+class ExplicitReceiverParameterEntity implements Local {
+  final String name;

[Kevin Millikin (Google), 2015/03/26 15:14:17]

Consider hardcoding the name 'receiver' here rather than at construction time.

+  final ExecutableElement executableContext;
+  ExplicitReceiverParameterEntity(this.name, this.executableContext);
+  toString() => 'ExplicitReceiverParameterEntity($executableContext)';
+}
+
+
+
 /// Rewrites the initial CPS IR to make Dart semantics explicit and inserts
 /// special nodes that respect JavaScript behavior.
 ///
 /// Performs the following rewrites:
-///  - rewrite [IsTrue] in a [Branch] to do boolean conversion.
+///  - Rewrite [IsTrue] in a [Branch] to do boolean conversion.
+///  - Add explicit receiver argument for methods that are called in interceptor
+///    calling convention.
+///  - Use current or new interceptor at call sites taht use interceptor calling

[Kevin Millikin (Google), 2015/03/26 15:14:17]

'taht' ==> 'that'

+///    convention.
+///  - Rewrite operator== to test the argument for null.

[Kevin Millikin (Google), 2015/03/26 15:14:17]

'first argument' or 'receiver argument'?

 class UnsugarVisitor extends RecursiveVisitor {
   Glue _glue;
 
+  bool inInterceptedMethod = false;
+  Parameter thisParameter = null;
+  Parameter explicitReceiverParameter = null;
+
   UnsugarVisitor(this._glue);
 
   void rewrite(FunctionDefinition function) {
+
+    if (function.element.name == '==' && function.parameters.length == 1) {
+      // If [functionElement] is `operator==` we explicitely add a null check at
+      // the beginning of the method. This is to avoid having call sites do the
+      // null check.
+      if (_glue.requiresEqNullCheck(function.element)) {
+        insertEqNullCheck(function);
+      }
+    }
+
+    // For interceptor calling convention, the receiver is passed as a
+    // parameter.
+    bool inInterceptedMethod = false;
+    if (_glue.isInterceptedMethod(function.element)) {
+      inInterceptedMethod = true;
+      thisParameter = function.thisParameter;
+      explicitReceiverParameter =
+          new Parameter(
+              new ExplicitReceiverParameterEntity('receiver',
+                  thisParameter.hint.executableContext));
+      function.parameters.insert(0, explicitReceiverParameter);
+    }
+
     // Set all parent pointers.
     new ParentVisitor().visit(function);
+
+    // Replace all references to This with the explicit receiver parameter.
+    if (inInterceptedMethod) {
+      explicitReceiverParameter.substituteFor(thisParameter);
+      explicitReceiverParameter.firstRef = thisParameter.firstRef;

[Kevin Millikin (Google), 2015/03/26 15:14:17]

substituteFor will do this (and doing it here is only correct in the case when
thisParameter.firstRef == null because we know there are no references to
explicitRecevierParameter).

+      thisParameter.firstRef = null;

[Kevin Millikin (Google), 2015/03/26 15:14:17]

And we should almost certainly do this in substituteFor as well.

I think this is the first time we're substituting x for y without removing y
from the CPS term, so we just haven't thought about the issue of what is seen as
uses of y after the substitution.

+    }
+
     visit(function);
   }
 
@@ -32,12 +86,24 @@ class UnsugarVisitor extends RecursiveVisitor {
     return result != null ? result : node;
   }
 
-  Constant get trueConstant {
+  Constant makeTrue() {

[Kevin Millikin (Google), 2015/03/26 15:14:17]

Oooh, these seem like good utilities to have in class ir.Constant.  What do you
think?

     return new Constant(
         new PrimitiveConstantExpression(
             new TrueConstantValue()));
   }
 
+  Constant makeFalse() {
+    return new Constant(
+        new PrimitiveConstantExpression(
+            new FalseConstantValue()));
+  }
+
+  Primitive makeNull() {
+    return new Constant(
+        new PrimitiveConstantExpression(
+            new NullConstantValue()));
+  }
+
   void insertLetPrim(Primitive primitive, Expression node) {
     LetPrim let = new LetPrim(primitive);
     InteriorNode parent = node.parent;
@@ -47,12 +113,65 @@ class UnsugarVisitor extends RecursiveVisitor {
     let.parent = parent;
   }
 
+  void insertEqNullCheck(FunctionDefinition function) {
+    // Replace
+    //
+    //     body;
+    //
+    // with
+    //
+    //     if (identical(arg, null))
+    //       return false;
+    //     else
+    //       body;
+    //
+    Continuation originalBody = new Continuation(<Parameter>[]);
+    originalBody.body = function.body.body;
+
+    Continuation returnFalse = new Continuation(<Parameter>[]);
+    Primitive falsePrimitive = makeFalse();
+    returnFalse.body =
+        new LetPrim(falsePrimitive,
+            new InvokeContinuation(
+                function.body.returnContinuation, <Primitive>[falsePrimitive]));
+
+    Primitive nullPrimitive = makeNull();
+    Primitive test = new Identical(function.parameters.single, nullPrimitive);
+
+    Expression newBody =
+        new LetCont.many(<Continuation>[returnFalse, originalBody],
+            new LetPrim(nullPrimitive,
+                new LetPrim(test,
+                    new Branch(
+                        new IsTrue(test),
+                        returnFalse,
+                        originalBody))));
+    function.body.body = newBody;
+  }
+
   processInvokeMethod(InvokeMethod node) {
     Selector selector = node.selector;
-    // TODO(karlklose):  should we rewrite all selectors?
+    // TODO(karlklose): should we rewrite all selectors?  sra: No.  It would add

[Kevin Millikin (Google), 2015/03/26 15:14:17]

I don't understand the question or the answer.  Maybe we three should agree on
what the question is, what the answer is, and get rid of the comment.

+    // a lot of code.  We would need to do so only if we permit native classes
+    // to implement noSuchMethod.
     if (!_glue.isInterceptedSelector(selector)) return;
 
     Primitive receiver = node.receiver.definition;
+
+    // If the receiver is the explicit receiver, we are calling a method in the
+    // same interceptor.
+    if (receiver == explicitReceiverParameter) {
+      //  receiver.foo()  -->  this.foo(receiver);
+      node.arguments.insert(0, node.receiver);
+      node.callingConvention = CallingConvention.JS_INTERCEPTED;
+      Reference<Primitive> receiverRef = node.receiver;

[Kevin Millikin (Google), 2015/03/26 15:14:17]

This is unused.

+      node.receiver = new Reference<Primitive>(thisParameter);
+      assert(node.isValid);
+      return;
+    }
+
+    // TODO(sra): Move the computation of interceptedClasses to a much later
+    // phase and take into account the remaining uses of the interceptor.
     Set<ClassElement> interceptedClasses =

[Kevin Millikin (Google), 2015/03/26 15:14:17]

I think there's enough shared code here and above that it could profitably be
something like:

Primitive newReceiver;
if (receiver == explicitReceiverParameter) {
  // If the receiver is the explicit receiver, we are calling a method in the
  // same interceptor.
  //   receiver.foo(...)  -->  this.foo(receiver, ...);
  newReceiver = thisParameter;
} else {
  // TODO(sra): Move the computation of interceptedClasses to a much later
  // phase and take into account the remaining uses of the interceptor.
  Set<ClassElement> interceptedClasses =
      _glue.getInterceptedClassesOn(selector);
  _glue.registerSpecializedGetInterceptor(interceptedClasses);
  newReceiver = new Interceptor(receiver, interceptedClasses);
  insertLetPrim(newReceiver, node);
}
node.arguments.insert(0, node.receiver);
node.callingConvention = CallingConvention.JS_INTERCEPTED;
node.receiver = new Reference<Primitive>(newReceiver);
assert(node.isValid);

         _glue.getInterceptedClassesOn(selector);
     _glue.registerSpecializedGetInterceptor(interceptedClasses);
@@ -61,13 +180,8 @@ class UnsugarVisitor extends RecursiveVisitor {
     insertLetPrim(intercepted, node);
     node.arguments.insert(0, node.receiver);
     node.callingConvention = CallingConvention.JS_INTERCEPTED;
-    assert(node.isValid);
     node.receiver = new Reference<Primitive>(intercepted);
-  }
-
-  Primitive makeNull() {
-    NullConstantValue nullConst = new NullConstantValue();
-    return new Constant(new PrimitiveConstantExpression(nullConst));
+    assert(node.isValid);
   }
 
   processInvokeMethodDirectly(InvokeMethodDirectly node) {
@@ -76,6 +190,12 @@ class UnsugarVisitor extends RecursiveVisitor {
       insertLetPrim(nullPrim, node);
       node.arguments.insert(0, node.receiver);
       node.receiver = new Reference<Primitive>(nullPrim);
+
+      // TODO(sra): `null` is not adequate.  Interceptors project the class
+      // hierarchy onto an interceptor hierarchy.  A super call that does a
+      // method call will use the javascript 'this' parameter to avoid calling
+      // getInterceptor again, so the receiver must be the interceptor (likely
+      // `this`), not `null`.
     }
   }
 
@@ -83,13 +203,15 @@ class UnsugarVisitor extends RecursiveVisitor {
     // TODO(karlklose): implement the checked mode part of boolean conversion.
     InteriorNode parent = node.parent;
     IsTrue condition = node.condition;
-    Primitive t = trueConstant;
+    Primitive t = makeTrue();
     Primitive i = new Identical(condition.value.definition, t);
-    LetPrim newNode = new LetPrim(t,
-        new LetPrim(i,
-            new Branch(new IsTrue(i),
-                node.trueContinuation.definition,
-                node.falseContinuation.definition)));
+    LetPrim newNode =
+        new LetPrim(t,
+            new LetPrim(i,
+                new Branch(
+                    new IsTrue(i),
+                    node.trueContinuation.definition,
+                    node.falseContinuation.definition)));
     condition.value.unlink();
     node.trueContinuation.unlink();
     node.falseContinuation.unlink();