dart2js cps: More aggressive operator specialization.

dart2js cps: More aggressive operator specialization.

Inline more unsafe operator calls, and insert more checks accordingly.

When "num" is the only receiver that does not respond with NSM, insert
a check and use the builtin operator.  Previously we only did this when
the receiver was a number or null. For example:

  if (typeof x !== "number") return x.$lt();
  result = x < 4;

When the argument might have the wrong type, insert an argument check
and use the builtin operator.  Previously we never did this if the
argument might have the wrong type. For example:

  if (typeof x !== "number") return H.iae(x);
  result = 4 < x;

If both receiver and argument need checks, we join the two checks and
use a one-shot interceptor to throw the error:

  if (typeof x !== "number" || typeof y !== "number")
    return J.$lt(x, y);
  result = x < y;

NullCheck has been generalized to a ReceiverCheck instruction to
support "x.$lt()" above, that throws even if x is a non-null value.

BUG=
R=sigmund@google.com

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

[asgerf, 2016-02-05 09:32:43]

asgerf@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2016-02-05 18:19:40]

lgtm!

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

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/backward_null_check_remover.dart:26: /// Note that
code motion may not occur after this pass, since the [ReceiverCheck]
nit: 80 col

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

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:263: /// If this is a
[Refinement], [BoundsCheck] or [ReceiverCheck] node, returns the
nit: 80 col

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:900: /// If [useSelector] is
false, the first form instead becomes `value.toString;`.
nit: since there are 2 groups, it's not entirely clear which one are you
referring to when you say 'first form' here. Maybe explicitly quote the form you
are referring to?

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

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:410: if (invoke.callingConvention ==
CallingConvention.OneShotIntercepted) {
Is this mainly for the one-shot interceptors you introduce in the
transformation? I wonder about whether this also has an effects on perf on its
own, in which case it might be worth submitting this line change on its own
first to know what is the baseline to compare against?

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

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1147: if (!hasReceiverGuard &&
!hasArgumentGuard) return cps;
nit: consider renaming hasReceiverGuard to needReceiverGuard & similarly
needArgumentGuard?

The reason is that this and the lines below on their own read sort-of backward:
I was confused for a moment thinking that if you don't have the guards, you need
to add them, so why return without doing so.

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1170: //   basic blocks.
+1 - I assume that would also help keep the instruction count lower for our
inlining heuristics?

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1457: if
(!lattice.isDefinitelyInt(getValue(index))) return null;
maybe add a TODO here and in []= to add argument checks to improve
specialization here too?

[asgerf, 2016-02-09 12:19:54]

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

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/backward_null_check_remover.dart:26: /// Note that
code motion may not occur after this pass, since the [ReceiverCheck]
On 2016/02/05 18:19:39, Siggi Cherem (dart-lang) wrote:
> nit: 80 col

Done.

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

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:263: /// If this is a
[Refinement], [BoundsCheck] or [ReceiverCheck] node, returns the
On 2016/02/05 18:19:39, Siggi Cherem (dart-lang) wrote:
> nit: 80 col

Done.

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:900: /// If [useSelector] is
false, the first form instead becomes `value.toString;`.
On 2016/02/05 18:19:39, Siggi Cherem (dart-lang) wrote:
> nit: since there are 2 groups, it's not entirely clear which one are you
> referring to when you say 'first form' here. Maybe explicitly quote the form
you
> are referring to?

Yeah that wasn't very clear. I've rephrased the comment.

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

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/inline.dart:410: if (invoke.callingConvention ==
CallingConvention.OneShotIntercepted) {
On 2016/02/05 18:19:39, Siggi Cherem (dart-lang) wrote:
> Is this mainly for the one-shot interceptors you introduce in the
> transformation? I wonder about whether this also has an effects on perf on its
> own, in which case it might be worth submitting this line change on its own
> first to know what is the baseline to compare against?

Regular one-shot interceptors are inserted later in the pipeline, so the only
one affected should be the one we used for bailout.  A more general solution is
to not inline code on throwing paths, which is on the TODO list.

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

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1147: if (!hasReceiverGuard &&
!hasArgumentGuard) return cps;
On 2016/02/05 18:19:40, Siggi Cherem (dart-lang) wrote:
> nit: consider renaming hasReceiverGuard to needReceiverGuard & similarly
> needArgumentGuard?
> 
> The reason is that this and the lines below on their own read sort-of
backward:
> I was confused for a moment thinking that if you don't have the guards, you
need
> to add them, so why return without doing so.

Good point. Done.

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1170: //   basic blocks.
On 2016/02/05 18:19:40, Siggi Cherem (dart-lang) wrote:
> +1 - I assume that would also help keep the instruction count lower for our
> inlining heuristics?

It's not clear if we should just count them as 1 instruction, though, because
the checks are pretty big, they're about 30 bytes each.

But it would certainly make the inlining heuristics easier to fine-tune since we
can assign a weight specifically to these checks.

https://codereview.chromium.org/1668913002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1457: if
(!lattice.isDefinitelyInt(getValue(index))) return null;
On 2016/02/05 18:19:39, Siggi Cherem (dart-lang) wrote:
> maybe add a TODO here and in []= to add argument checks to improve
> specialization here too?

Done.  I'll try it out separately.

[asgerf, 2016-02-09 12:34:41]

Description was changed from

==========
dart2js cps: More aggressive operator specialization.

Inline more unsafe operator calls, and insert more checks accordingly.

When "num" is the only receiver that does not respond with NSM, insert
a check and use the builtin operator.  Previously we only did this when
the receiver was a number or null. For example:

  if (typeof x !== "number") return x.$lt();
  result = x < 4;

When the argument might have the wrong type, insert an argument check
and use the builtin operator.  Previously we never did this if the
argument might have the wrong type. For example:

  if (typeof x !== "number") return H.iae(x);
  result = 4 < x;

If both receiver and argument need checks, we join the two checks and
use a one-shot interceptor to throw the error:

  if (typeof x !== "number" || typeof y !== "number")
    return J.$lt(x, y);
  result = x < y;

NullCheck has been generalized to a ReceiverCheck instruction to
support "x.$lt()" above, that throws even if x is a non-null value.

BUG=
==========

to

==========
dart2js cps: More aggressive operator specialization.

Inline more unsafe operator calls, and insert more checks accordingly.

When "num" is the only receiver that does not respond with NSM, insert
a check and use the builtin operator.  Previously we only did this when
the receiver was a number or null. For example:

  if (typeof x !== "number") return x.$lt();
  result = x < 4;

When the argument might have the wrong type, insert an argument check
and use the builtin operator.  Previously we never did this if the
argument might have the wrong type. For example:

  if (typeof x !== "number") return H.iae(x);
  result = 4 < x;

If both receiver and argument need checks, we join the two checks and
use a one-shot interceptor to throw the error:

  if (typeof x !== "number" || typeof y !== "number")
    return J.$lt(x, y);
  result = x < y;

NullCheck has been generalized to a ReceiverCheck instruction to
support "x.$lt()" above, that throws even if x is a non-null value.

BUG=
R=sigmund@google.com

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

[asgerf, 2016-02-09 12:34:43]

Committed patchset #4 (id:60001) manually as
bbca76a5e2df6c5adf0ee9327f3a7af21fb1c19a (presubmit successful).