cpsir: insert guard and force specialization for [] and []=

cpsir: insert guard and force specialization for [] and []=

R=asgerf@google.com

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

[Siggi Cherem (dart-lang), 2016-02-09 22:10:54]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2016-02-09 22:11:03]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2016-02-09 22:12:38]

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

[asgerf, 2016-02-10 08:37:04]

https://codereview.chromium.org/1685893002/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/builtin_operator.dart (right):

https://codereview.chromium.org/1685893002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:136: IsNumberAndFloor,
Please rename this to IsInteger to be consistent.

https://codereview.chromium.org/1685893002/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1685893002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1477: // Insert a guard and
specialize anyways.
In case the array is null, we will now throw an ArgumentError instead of a
NoSuchMethodError.

The GetLength created by makeBoundsCheck is the effective the null guard. We
could create the GetLength first, then do the argument check, and then do the
bounds check using the existing GetLength.

https://codereview.chromium.org/1685893002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:3444: ? [value, value, value]
: [value]);
For integers, we can omit the floor test if the value is an integer or null, and
we can omit the typeof test if it is known to be a number.

visitTypeTest has this logic already. Maybe it's better to just insert a
TypeTest node?

I did not do that originally because I was worried about the cost of repeated
reprocessing, but I don't know how expensive it really is.

[Siggi Cherem (dart-lang), 2016-02-12 00:10:12]

Patchset #2 (id:60001) has been deleted

[Siggi Cherem (dart-lang), 2016-02-12 00:12:44]

Patchset #2 (id:80001) has been deleted

[Siggi Cherem (dart-lang), 2016-02-12 00:14:00]

Thanks Asger, PTAL

https://codereview.chromium.org/1685893002/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/builtin_operator.dart (right):

https://codereview.chromium.org/1685893002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:136: IsNumberAndFloor,
On 2016/02/10 08:37:04, asgerf wrote:
> Please rename this to IsInteger to be consistent.

Technically I no longer need IsNotInteger, so I could remove it, but I left it
here (and rename this to IsInteger) so we get the negated expression from the
tree-ir logical tree rewrite.

Is it worth it? I'm happy to remove it if you prefer.

https://codereview.chromium.org/1685893002/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1685893002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1477: // Insert a guard and
specialize anyways.
On 2016/02/10 08:37:04, asgerf wrote:
> In case the array is null, we will now throw an ArgumentError instead of a
> NoSuchMethodError.
> 
> The GetLength created by makeBoundsCheck is the effective the null guard. We
> could create the GetLength first, then do the argument check, and then do the
> bounds check using the existing GetLength.

good catch. Done

https://codereview.chromium.org/1685893002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:3444: ? [value, value, value]
: [value]);
On 2016/02/10 08:37:04, asgerf wrote:
> For integers, we can omit the floor test if the value is an integer or null,
and
> we can omit the typeof test if it is known to be a number.
> 
> visitTypeTest has this logic already. Maybe it's better to just insert a
> TypeTest node?
> 
> I did not do that originally because I was worried about the cost of repeated
> reprocessing, but I don't know how expensive it really is.

I see - good point. I replaced it with the type test, I'll try to measure to see
what the cost is, but I don't expect a big difference.

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

https://codereview.chromium.org/1685893002/diff/100001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1494: if
(compiler.trustPrimitives ||
If we trust-primitives, we can trust that the index is int too :)

[asgerf, 2016-02-12 10:29:10]

LGTM

https://codereview.chromium.org/1685893002/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/builtin_operator.dart (right):

https://codereview.chromium.org/1685893002/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/builtin_operator.dart:136: IsNumberAndFloor,
On 2016/02/12 00:14:00, Siggi Cherem (dart-lang) wrote:
> On 2016/02/10 08:37:04, asgerf wrote:
> > Please rename this to IsInteger to be consistent.
> 
> Technically I no longer need IsNotInteger, so I could remove it, but I left it
> here (and rename this to IsInteger) so we get the negated expression from the
> tree-ir logical tree rewrite.
> 
> Is it worth it? I'm happy to remove it if you prefer.

No please leave it in. It's been bothering me that we could not negate it.

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

https://codereview.chromium.org/1685893002/diff/100001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1446: cps.ifTruthy(
Please use cps.ifFalsy instead of cps.ifTruthy(IsFalsy(x))

[Siggi Cherem (dart-lang), 2016-02-12 17:21:56]

Description was changed from

==========
cpsir: insert guard and force specialization for [] and []=
==========

to

==========
cpsir: insert guard and force specialization for [] and []=

R=asgerf@google.com

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

[Siggi Cherem (dart-lang), 2016-02-12 17:21:57]

Committed patchset #3 (id:120001) manually as
48b0df5fa1b0ec289fbc7f2444f519a5124f055e (presubmit successful).

[Siggi Cherem (dart-lang), 2016-02-12 17:21:58]

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

https://codereview.chromium.org/1685893002/diff/100001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1446: cps.ifTruthy(
On 2016/02/12 10:29:10, asgerf wrote:
> Please use cps.ifFalsy instead of cps.ifTruthy(IsFalsy(x))

<facepalm> - thanks, all done.