dart2js cps_ir: Use interceptors for is-checks (version 2)

Use interceptors for is-checks (version 2).

Use the $isXXX properties for type tests on raw (non-parameterized) types.

Use special builtin operations for the weird JSArray type hierarchy.

Still TODO:

If the only use of an interceptor is a single 'getInterceptor(value).$isXXX', and XXX is not a supertype of a native type, replace with 'value instanceof XXX'.

R=kmillikin@google.com

Committed: https://github.com/dart-lang/sdk/commit/3b4e12f00bbdf2f5c16ca99c28300ebb63f794a9

Reverted: https://github.com/dart-lang/sdk/commit/89b6d8e74960dec2fd87011538227ab515d0fc8d

Committed: https://github.com/dart-lang/sdk/commit/5265eeebaac65decca1b2ce489c7d12619ef82b3

[sra1, 2015-10-07 03:47:01]

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

[sra1, 2015-10-07 04:01:29]

This replaces https://codereview.chromium.org/1388473003/

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart (right):

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart:352: return "TypeTest ($value
${node.dartType} ($args)$interceptor)";
How is this tested?

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart:357: return "TypeTestRaw
($interceptor ${node.dartType})";
TypeTestRaw -> TypeTestViaFlag

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1818: }
If instead of removing the interceptor, we add one if needed, then we are
generating

   letPrim p1 = TypeTest x in
     ...

->

   letPrim p2 = Interceptor x in
     letPrim p1 = TypeTestViaFlag p2 in
       ...

I'm not sure how to so that given this visitor method is called from
visitLetPrim for p1.  It seems that I can't get reanalyze to start from the
right place.

Perhaps a more general way to handle primitives is for each visitor to finish
the job by tail-calling into code equivalent to the continuation of visit() in
visitLetPrim when there is a single replacement.

The other thing I want to do is rewrite some tests as control flow.
Sometimes we can replace

  if (getInterceptor(x).$isFoo) ...

with

  if (x != null && x.$isFoo) ...

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2313: break;
What to I do for primitives that return JS values that only make sense in a
truthy/falsy context?

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/tree_i...
File pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart (right):

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart:507: return
'$object."is-${node.dartType}"';
How do I test this?

[Kevin Millikin (Google), 2015-10-07 14:10:50]

Comments below.  I wonder if this is the sort of thing we want to move out of
type propagation.

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:590: 
Not sure why there's a space here (there isn't one in the parallel class
TypeTestViaFlag below).

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:593: /// If [type] is an
[InterfaceType], this holds the internal representation of
Hmm, [type] in the doc comment didn't get renamed when type was renamed to
dartType.  Could you add that to this change since you're touching the code?

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:606: Reference<Primitive>
interceptor;
Needs some documentation: how do I interpret a non-null interceptor?  A null
interceptor?

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (right):

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:246: String interceptor =
node.interceptor == null
We've sort of given up on parsing the S-expressions for now, but for the future,
we should probably make the optional part surrounded by parentheses and always
there:

String interceptor = node.interceptor == null
    ? ''
    : access(node.interceptor);

and with " ($interceptor)" in the returned string below.

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart (right):

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart:352: return "TypeTest ($value
${node.dartType} ($args)$interceptor)";
On 2015/10/07 04:01:28, sra1 wrote:
> How is this tested?

Manually.  Set the string constant DUMP_IR in the environment, it's a pattern
that's used to construct a regexp that matches functions that will be dumped. 
So if you just set the environment variable DART_VM_OPTIONS="-DDUMP_IR=" and run
the dart2js script then that will match everything because it's an empty pattern
string.

Dart2js will write out a file named dart.cfg (this filename is hardcoded) in the
working directory.

Go to http://mrale.ph/irhydra/2/ and click on the file folder in the upper
left-hand corner.  Open the dart.cfg file and inspect it.

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/type_mask_system.dart (right):

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_mask_system.dart:68: interceptorType = new
TypeMask.nonNullSubtype(
I guess all the lines above are broken like

interceptorType =
    new TypeMask.nonNullSubtype(backend.jsInterceptorClass, classWorld);

which I think looks a bit better (broken at a lower precedence operator, avoids
the unsightly dangling left paren).

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1818: }
Yeah, that corresponds to replacing a single instruction with a pair of
instructions.  Since this pass was originally sparse conditional constant
propagation, we didn't really build it so that was easy.

I wonder if we really want to do that in this pass anyway?  At least in this
case, it seems like only two of the transformations have anything to do with the
type computed for 'prim' --- the one that replace an 'is int' check with
Math.floor when prim is known to be a number, and the one that replaces an 'is
int' check with typeof === 'number' when prim is known to not be a double.

It seems like the rest of the transformations could either be done at
construction time (if they don't occur due to other optimizations) or else as a
separate pass run before this one or as part of shrinking reductions (if the
pattern can occur as a result of other optimizations).  Is there any
optimization that changes the dartType of an TypeTest?

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2313: break;
On 2015/10/07 04:01:28, sra1 wrote:
> What to I do for primitives that return JS values that only make sense in a
> truthy/falsy context?

If they're constant values, they should be the appropriate constant.  If they
are non-constant values they should have the appropriate type.  Then we need to
make sure that ConstantPropagationLattice.boolify gives the right answer for
them.

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/tree_i...
File pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart (right):

https://codereview.chromium.org/1385423002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart:507: return
'$object."is-${node.dartType}"';
On 2015/10/07 04:01:28, sra1 wrote:
> How do I test this?

Manually.  This will also be written to dart.cfg when you set DUMP_IR in the
environment.

[Kevin Millikin (Google), 2015-10-07 16:12:07]

LGTM with the small cleanups I noted.  We can postpone the decision whether to
move it out of type propagation until later.

[sra1, 2015-10-07 17:45:58]

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:590: 
On 2015/10/07 14:10:49, Kevin Millikin (Google) wrote:
> Not sure why there's a space here (there isn't one in the parallel class
> TypeTestViaFlag below).

Done.

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:593: /// If [type] is an
[InterfaceType], this holds the internal representation of
On 2015/10/07 14:10:49, Kevin Millikin (Google) wrote:
> Hmm, [type] in the doc comment didn't get renamed when type was renamed to
> dartType.  Could you add that to this change since you're touching the code?

Done.

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:606: Reference<Primitive>
interceptor;
On 2015/10/07 14:10:49, Kevin Millikin (Google) wrote:
> Needs some documentation: how do I interpret a non-null interceptor?  A null
> interceptor?

Done.

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (right):

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:246: String interceptor =
node.interceptor == null
On 2015/10/07 14:10:49, Kevin Millikin (Google) wrote:
> We've sort of given up on parsing the S-expressions for now, but for the
future,
> we should probably make the optional part surrounded by parentheses and always
> there:
> 
> String interceptor = node.interceptor == null
>     ? ''
>     : access(node.interceptor);
> 
> and with " ($interceptor)" in the returned string below.

Done.

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart (right):

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart:357: return "TypeTestRaw
($interceptor ${node.dartType})";
On 2015/10/07 04:01:28, sra1 wrote:
> TypeTestRaw -> TypeTestViaFlag

Done.

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/cps_ir/type_mask_system.dart (right):

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/type_mask_system.dart:68: interceptorType = new
TypeMask.nonNullSubtype(
On 2015/10/07 14:10:49, Kevin Millikin (Google) wrote:
> I guess all the lines above are broken like
> 
> interceptorType =
>     new TypeMask.nonNullSubtype(backend.jsInterceptorClass, classWorld);
> 
> which I think looks a bit better (broken at a lower precedence operator,
avoids
> the unsightly dangling left paren).

Done.

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1818: }
On 2015/10/07 14:10:49, Kevin Millikin (Google) wrote:
> Yeah, that corresponds to replacing a single instruction with a pair of
> instructions.  Since this pass was originally sparse conditional constant
> propagation, we didn't really build it so that was easy.
> 
> I wonder if we really want to do that in this pass anyway?  At least in this
> case, it seems like only two of the transformations have anything to do with
the
> type computed for 'prim' --- the one that replace an 'is int' check with
> Math.floor when prim is known to be a number, and the one that replaces an 'is
> int' check with typeof === 'number' when prim is known to not be a double.
> 
> It seems like the rest of the transformations could either be done at
> construction time (if they don't occur due to other optimizations) or else as
a
> separate pass run before this one or as part of shrinking reductions (if the
> pattern can occur as a result of other optimizations).  Is there any
> optimization that changes the dartType of an TypeTest?

The dartType never changes.  The rewrite depends on both the dartType and the
input type.

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart (right):

https://chromiumcodereview.appspot.com/1385423002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/tree_ir/tree_ir_tracer.dart:507: return
'$object."is-${node.dartType}"';
On 2015/10/07 14:10:49, Kevin Millikin (Google) wrote:
> On 2015/10/07 04:01:28, sra1 wrote:
> > How do I test this?
> 
> Manually.  This will also be written to dart.cfg when you set DUMP_IR in the
> environment.

Acknowledged.

[sra1, 2015-10-07 22:04:50]

Patchset #2 (id:20001) has been deleted

[sra1, 2015-10-07 22:47:37]

Committed patchset #2 (id:40001) manually as
3b4e12f00bbdf2f5c16ca99c28300ebb63f794a9 (presubmit successful).

[sra1, 2015-10-08 00:07:11]

Committed patchset #4 (id:80001) manually as
5265eeebaac65decca1b2ce489c7d12619ef82b3 (presubmit successful).