cps_ir: interceptor calling convention, operator== prologue

Methods using interceptor calling convention take an explicit interceptor argument.

operator== checks argument against null

Closed this issue - the work has been committed as part of another CL

[sra1, 2015-03-23 19:06:40]

sra@google.com changed reviewers:
+ kmillikin@google.com

[sra1, 2015-03-23 21:00:35]

Patchset #1 (id:1) has been deleted

[sra1, 2015-03-23 21:24:41]

The generated code for operator== looks like:

    $eq: function(receiver, other) {
      var v0, v1, ...;
      if (P.identical(P.identical(other, null), true))
        return false;
      else {
        ...
      }
    },

I believe I am generating a term that looks more like:

    $eq: function(receiver, other) {
      var v0, v1, ...;
      if (P.identical(P.identical(other, null), true))
        return false;
      ...
    },

Shrinking reductions rewrites to the first form which takes more space when
minified due to the extra 'else {' and '}'.

[asgerf, 2015-03-24 09:03:41]

On 2015/03/23 21:24:41, sra1 wrote:
> The generated code for operator== looks like:
> 
>     $eq: function(receiver, other) {
>       var v0, v1, ...;
>       if (P.identical(P.identical(other, null), true))
>         return false;
>       else {
>         ...
>       }
>     },
> 
> I believe I am generating a term that looks more like:
> 
>     $eq: function(receiver, other) {
>       var v0, v1, ...;
>       if (P.identical(P.identical(other, null), true))
>         return false;
>       ...
>     },
> 
> Shrinking reductions rewrites to the first form which takes more space when
> minified due to the extra 'else {' and '}'.

In the tree language, we are going to eliminate 'else' when the 'then' part
cannot fallthrough. So it's not a problem with the CPS code.

[Kevin Millikin (Google), 2015-03-24 10:27:47]

On 2015/03/24 09:03:41, asgerf wrote:

> In the tree language, we are going to eliminate 'else' when the 'then' part
> cannot fallthrough. So it's not a problem with the CPS code.

So: you should actually generate the eta-reduced code in the first place since
we'll eta-reduce it anyway.

[sra1, 2015-03-26 01:16:57]

On 2015/03/24 10:27:47, kmillikin wrote:
> On 2015/03/24 09:03:41, asgerf wrote:
> 
> > In the tree language, we are going to eliminate 'else' when the 'then' part
> > cannot fallthrough. So it's not a problem with the CPS code.
> 
> So: you should actually generate the eta-reduced code in the first place since
> we'll eta-reduce it anyway.

Done

[sra1, 2015-03-26 01:17:38]

PTAL.

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

LGTM with a bunch of suggestions and a few things to fix.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2475: Selector selector =
new Selector.fromElement(element);
Inadvertent change?  selector is only used in the commented-out code below.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/glue.dart (right):

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/glue.dart:158: bool
requiresEqNullCheck(FunctionElement element) {
Can we avoid having two different names (requiresEqNullCheck vs.
!operatorEqHandlesNullArgument) for the same thing in two different places?

If one is obviously better, we should just adopt it in both places.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/unsugar.dart (right):

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:19: final String name;
Consider hardcoding the name 'receiver' here rather than at construction time.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:34: ///  - Use current or
new interceptor at call sites taht use interceptor calling
'taht' ==> 'that'

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:36: ///  - Rewrite
operator== to test the argument for null.
'first argument' or 'receiver argument'?

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:76:
explicitReceiverParameter.firstRef = thisParameter.firstRef;
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).

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:77: thisParameter.firstRef
= null;
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.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:89: Constant makeTrue() {
Oooh, these seem like good utilities to have in class ir.Constant.  What do you
think?

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:154: // TODO(karlklose):
should we rewrite all selectors?  sra: No.  It would add
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.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:167: Reference<Primitive>
receiverRef = node.receiver;
This is unused.

https://codereview.chromium.org/1020243002/diff/60001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:175: Set<ClassElement>
interceptedClasses =
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);