dart2js cps: Add instruction for bounds checks.

dart2js cps: Add instruction for bounds checks.

The new instruction is expanded into low-level operations
in a new pass Finalize. The Finalize pass is mandatory.

The BoundsCheck returns the indexable object being checked
so it can be used to restrict code motion.

BUG=
R=sra@google.com

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

[asgerf, 2015-12-10 18:29:03]

Description was changed from

==========
dart3js cps: Add a BoundsCheck instruction.

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

to

==========
dart2js cps: Add a BoundsCheck instruction.

The new instruction is expanded into low-level operations
in a new pass Finalize. The Finalize pass is mandatory.

The BoundsCheck returns the indexable object being checked
so it can be used to restrict code motion.

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

[asgerf, 2015-12-10 18:29:37]

Patchset #2 (id:20001) has been deleted

[asgerf, 2015-12-10 18:30:05]

Description was changed from

==========
dart2js cps: Add a BoundsCheck instruction.

The new instruction is expanded into low-level operations
in a new pass Finalize. The Finalize pass is mandatory.

The BoundsCheck returns the indexable object being checked
so it can be used to restrict code motion.

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

to

==========
dart2js cps: Add instruction for bounds checks.

The new instruction is expanded into low-level operations
in a new pass Finalize. The Finalize pass is mandatory.

The BoundsCheck returns the indexable object being checked
so it can be used to restrict code motion.

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

[asgerf, 2015-12-10 18:32:13]

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

[asgerf, 2015-12-10 18:32:14]

https://codereview.chromium.org/1512303002/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/codegen/task.dart (right):

https://codereview.chromium.org/1512303002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/task.dart:213: applyCpsPass(new
BoundsChecker(typeSystem, compiler.world), cpsFunction);
It works better with refinement information. We might want to move it down again
when we retain refinement in the whole pipeline.

[sra1, 2015-12-10 19:09:40]

lgtm

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

https://codereview.chromium.org/1512303002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:754: /// True of the [length] is
needed.
... is needed to perform the check.

Maybe call it 'lengthUsedInCheck'.

EMPTINESS uses node.length.definition in Finalize.

https://codereview.chromium.org/1512303002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:761: static const int EMPTINESS =
1 << 2;
What is EMPTINESS?

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

https://codereview.chromium.org/1512303002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/finalize.dart:67: .invokeContinuation(fail);
(a.length === 0)

I think this could be (!a.length) in many cases where we can expect the JS
engine to know the type, e.g. the length is GetLength

[asgerf, 2015-12-11 12:16:31]

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

https://codereview.chromium.org/1512303002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:754: /// True of the [length] is
needed.
On 2015/12/10 19:09:40, sra1 wrote:
> ... is needed to perform the check.
> 
> Maybe call it 'lengthUsedInCheck'.
> 
> EMPTINESS uses node.length.definition in Finalize.

Done.

https://codereview.chromium.org/1512303002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:761: static const int EMPTINESS =
1 << 2;
On 2015/12/10 19:09:39, sra1 wrote:
> What is EMPTINESS?

I thought it would be clear from hasEmptinessCheck. I added a comment
redirecting there, and made it more clear in the doc for BoundsCheck that there
is a third kind of bounds check.

It is used to generate pop():

y = xs.removeLast()

==>

if (xs.length === 0) return ioore(xs, -1);
var y = xs.pop()

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

https://codereview.chromium.org/1512303002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/finalize.dart:67: .invokeContinuation(fail);
On 2015/12/10 19:09:40, sra1 wrote:
> (a.length === 0)
> 
> I think this could be (!a.length) in many cases where we can expect the JS
> engine to know the type, e.g. the length is GetLength

I had to resolve to micro benchmarking to measure this, but FWIW "!a.length" is
measurably slower in a micro benchmark, even when it knows a.length is a SMI.

Interestingly, it seems V8 keeps the bounds check for .pop() no matter what we
put in front of it, but the check itself is just faster with === 0.

[asgerf, 2015-12-14 13:41:53]

Description was changed from

==========
dart2js cps: Add instruction for bounds checks.

The new instruction is expanded into low-level operations
in a new pass Finalize. The Finalize pass is mandatory.

The BoundsCheck returns the indexable object being checked
so it can be used to restrict code motion.

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

to

==========
dart2js cps: Add instruction for bounds checks.

The new instruction is expanded into low-level operations
in a new pass Finalize. The Finalize pass is mandatory.

The BoundsCheck returns the indexable object being checked
so it can be used to restrict code motion.

BUG=
R=sra@google.com

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

[asgerf, 2015-12-14 13:41:54]

Committed patchset #3 (id:60001) manually as
f53ce36c3d260710001d4a6f080f5e7cd83624f4 (presubmit successful).