dart2js cps: More interceptor optimizations and fixes.

dart2js cps: More interceptor optimizations and fixes.

When the result of getInterceptor is either null or a known constant,
it is compiled to `x && const` or `x == null ? x : const` depending
on whether there are any other falsy values for that type.

This required some changes to share_interceptors so it can share/hoist
the constant subexpression separately from the whole expression.

The Interceptor root class is removed from the intercepted classes of
getInterceptor when it is "shadowed" by all the other intercepted classes.
E.g. if the input is a number or a string, there is no need
to intercept for {JSNumber, JSString, Interceptor}, because JSNumber
and JSString collectively shadow Interceptor.

R=sra@google.com

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

[asgerf, 2015-10-20 18:13:48]

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

[sra1, 2015-10-21 00:46:25]

lgtm

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

https://chromiumcodereview.appspot.com/1409803003/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/share_interceptors.dart:85: // There might exist
self-intercepting subclasses of native classes.
I don't think this is true.  Custom elements should be intercepted.

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

https://chromiumcodereview.appspot.com/1409803003/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2201: bool catchNull =
'include' might be a better name than 'catch'.

https://chromiumcodereview.appspot.com/1409803003/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2207:
node.interceptedClasses.add(backend.jsIntClass);
Maybe:

node.interceptedClasses..clear()..add(backend.jsIntClass);

[sra1, 2015-10-21 01:29:39]

No longer lgtm.

This is a bug (in swarm):

        if (J.getInterceptor(receiver).$isBodyElement)
          contextElement = $.Element__parseDocument.body;
-->
        if (C.Interceptor_methods.$isBodyElement)
          contextElement = $.Element__parseDocument.body;

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

https://chromiumcodereview.appspot.com/1409803003/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/share_interceptors.dart:85: // There might exist
self-intercepting subclasses of native classes.
On 2015/10/21 00:46:25, sra1 wrote:
> I don't think this is true.  Custom elements should be intercepted.

Specifically, this is an important regression:


      $index: function(receiver, index) {
        return C.NodeList_methods.$index(this._nodeList, index);
      },
-->
      $index: function(receiver, index) {
        var v0 = this._nodeList;
        return J.getInterceptor$x(v0).$index(v0, index);
      },

[asgerf, 2015-10-21 11:52:47]

Thanks for catching, PTAL.

I had misunderstood how the intercepted class set was used in specialized
interceptors.

There are now separate flags for "exact intercept" where the value's
class is exactly one of the intercepted classes.

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

https://chromiumcodereview.appspot.com/1409803003/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/share_interceptors.dart:85: // There might exist
self-intercepting subclasses of native classes.
On 2015/10/21 01:29:38, sra1 wrote:
> On 2015/10/21 00:46:25, sra1 wrote:
> > I don't think this is true.  Custom elements should be intercepted.
> 
> Specifically, this is an important regression:
> 
> 
>       $index: function(receiver, index) {
>         return C.NodeList_methods.$index(this._nodeList, index);
>       },
> -->
>       $index: function(receiver, index) {
>         var v0 = this._nodeList;
>         return J.getInterceptor$x(v0).$index(v0, index);
>       },

Should be fixed now.

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

https://chromiumcodereview.appspot.com/1409803003/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2201: bool catchNull =
On 2015/10/21 00:46:25, sra1 wrote:
> 'include' might be a better name than 'catch'.

Changed to 'interceptNull'. Hope that's better.

https://chromiumcodereview.appspot.com/1409803003/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2207:
node.interceptedClasses.add(backend.jsIntClass);
On 2015/10/21 00:46:25, sra1 wrote:
> Maybe:
> 
> node.interceptedClasses..clear()..add(backend.jsIntClass);

Done.

[sra1, 2015-10-21 21:46:56]

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

https://chromiumcodereview.appspot.com/1409803003/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/share_interceptors.dart:85: // There might exist
self-intercepting subclasses of native classes.
On 2015/10/21 11:52:46, asgerf wrote:
> On 2015/10/21 01:29:38, sra1 wrote:
> > On 2015/10/21 00:46:25, sra1 wrote:
> > > I don't think this is true.  Custom elements should be intercepted.
> > 
> > Specifically, this is an important regression:
> > 
> > 
> >       $index: function(receiver, index) {
> >         return C.NodeList_methods.$index(this._nodeList, index);
> >       },
> > -->
> >       $index: function(receiver, index) {
> >         var v0 = this._nodeList;
> >         return J.getInterceptor$x(v0).$index(v0, index);
> >       },
> 
> Should be fixed now.

I'm still seeing some cases of regressions:

      eraseSplashScreen$0: function() {
        var splash = C.HtmlDocument_methods.querySelector$1(document,
"#appSplash");
        if (splash)
          J.getInterceptor$x(splash).remove$0(splash);
      },
-->
      eraseSplashScreen$0: function() {
        var v0 = document, splash = J.getInterceptor$x(v0).querySelector$1(v0,
"#appSplash");
        if (splash)
          J.getInterceptor$x(splash).remove$0(splash);
      },

https://chromiumcodereview.appspot.com/1409803003/diff/40001/pkg/compiler/lib...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (left):

https://chromiumcodereview.appspot.com/1409803003/diff/40001/pkg/compiler/lib...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2902: 
Keep this blank line

[asgerf, 2015-10-23 15:29:24]

Sorry for all the bugs and the messy patch history. This time I really think it
should be good.

I wanted to move the interceptor optimization later in pipeline, but decided to
postpone it. It should be easier after the refactoring.

[sra1, 2015-10-28 02:31:53]

lgtm

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart (right):

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart:329:
"${node.interceptedClasses})";
use single quotes unless string contains single quotes

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/share_interceptors.dart (right):

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/share_interceptors.dart:258:
existing.useElementAsHint(prim.hint);
Should we really keep the dominating hint in the case of a conflict?
In the following, neither x_ nor y_ are good names:

var x_ = C.JSNumber_methods, y_ = C.JSNumber_methods;
x_.toInt$0(x)
y_.toInt$0(y)
->
var x_ = C.JSNumber_methods;
x_.toInt$0(x);
x_.toInt$0(y);

In this case we should probably make a hint based on the class, e.g. JSNumber_
or num_

These comments are FYI. We can address this later. I think some hints could be
better chosen in a specific pass.

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2238: // will hit one of the
more specific classes.
Can you give an example?

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:1021: return class_ ==
jsPositiveIntClass ||
indent

[asgerf, 2015-10-28 10:06:31]

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart (right):

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart:329:
"${node.interceptedClasses})";
On 2015/10/28 02:31:52, sra1 wrote:
> use single quotes unless string contains single quotes

Done.

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/share_interceptors.dart (right):

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/share_interceptors.dart:258:
existing.useElementAsHint(prim.hint);
On 2015/10/28 02:31:52, sra1 wrote:
> Should we really keep the dominating hint in the case of a conflict?
> In the following, neither x_ nor y_ are good names:
> 
> var x_ = C.JSNumber_methods, y_ = C.JSNumber_methods;
> x_.toInt$0(x)
> y_.toInt$0(y)
> ->
> var x_ = C.JSNumber_methods;
> x_.toInt$0(x);
> x_.toInt$0(y);
> 
> In this case we should probably make a hint based on the class, e.g. JSNumber_
> or num_
> 
> These comments are FYI. We can address this later. I think some hints could be
> better chosen in a specific pass.

I agree, the names usually aren't that great, and sometimes downright confusing.

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2238: // will hit one of the
more specific classes.
On 2015/10/28 02:31:52, sra1 wrote:
> Can you give an example?

Done.

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/j...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/1409803003/diff/140001/pkg/compiler/lib/src/j...
pkg/compiler/lib/src/js_backend/backend.dart:1021: return class_ ==
jsPositiveIntClass ||
On 2015/10/28 02:31:52, sra1 wrote:
> indent

Done.

[asgerf, 2015-10-28 11:56:26]

Committed patchset #11 (id:200001) manually as
f712e08e949f39f72fe7a446995f5b7dbf6f590d (presubmit successful).