dart2js cps: Disable RHS propagation in operands to builtin operators.

dart2js cps: Disable RHS propagation in operands to builtin operators.

Previously we would get code like `x == (y = null)`, which confuses
the JS engine.  It is better to have `y = null` and `x == null` separate.

BUG=
R=sra@google.com

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

[asgerf, 2016-01-06 00:18:41]

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

[asgerf, 2016-01-06 00:18:41]

https://codereview.chromium.org/1563693002/diff/1/pkg/compiler/lib/src/tree_i...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (left):

https://codereview.chromium.org/1563693002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:196: 
Drive-by cleanup

[sra1, 2016-01-06 00:41:06]

lgtm

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

https://codereview.chromium.org/1563693002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:199: ///
runtime so it is disabled in some cases.
'engine' rather than 'runtime'.

https://codereview.chromium.org/1563693002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:200:
List<bool> allowRhsPropagation = <bool>[true];
I assume it was too nasty to thread through as an argument.

https://codereview.chromium.org/1563693002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:202: bool get
isAllowingRhsPropagation => allowRhsPropagation.last;
Maybe rename to isRhsPropagationAllowed.
('are you allowing ...' vs 'is it allowed ...')

https://codereview.chromium.org/1563693002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:906:
_rewriteList(node.arguments, rhsPropagation: false);
Add a comment why rhsPropagation is false.

    if (p == (q.f = null)) ...

is slow on V8.

The problem is only with V8, only when the operation is == (!=), and only when
one operand is `null`.
V8 generates efficient code for `x == null` only when `null` is syntactically an
operand, otherwise it is very slow.

[asgerf, 2016-01-06 01:26:14]

Description was changed from

==========
dart2js cps: Disable RHS propagation in operands to builtin operators.

Previously we would code like `x == (y = null)`, which confuses the
JS runtime.  It is better to have `y = null` and `x == null` separate.

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

to

==========
dart2js cps: Disable RHS propagation in operands to builtin operators.

Previously we would get code like `x == (y = null)`, which confuses
the JS engine.  It is better to have `y = null` and `x == null` separate.

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

[asgerf, 2016-01-06 15:43:31]

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

https://codereview.chromium.org/1563693002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:199: ///
runtime so it is disabled in some cases.
On 2016/01/06 00:41:06, sra1 wrote:
> 'engine' rather than 'runtime'.

Done.

https://codereview.chromium.org/1563693002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:200:
List<bool> allowRhsPropagation = <bool>[true];
On 2016/01/06 00:41:06, sra1 wrote:
> I assume it was too nasty to thread through as an argument.

Yeah.

https://codereview.chromium.org/1563693002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:202: bool get
isAllowingRhsPropagation => allowRhsPropagation.last;
On 2016/01/06 00:41:06, sra1 wrote:
> Maybe rename to isRhsPropagationAllowed.
> ('are you allowing ...' vs 'is it allowed ...')

Done.

https://codereview.chromium.org/1563693002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:906:
_rewriteList(node.arguments, rhsPropagation: false);
On 2016/01/06 00:41:06, sra1 wrote:
> Add a comment why rhsPropagation is false.
> 
>     if (p == (q.f = null)) ...
> 
> is slow on V8.
> 
> The problem is only with V8, only when the operation is == (!=), and only when
> one operand is `null`.
> V8 generates efficient code for `x == null` only when `null` is syntactically
an
> operand, otherwise it is very slow.
> 
> 
> 
> 

Done.

[asgerf, 2016-01-06 15:44:07]

Description was changed from

==========
dart2js cps: Disable RHS propagation in operands to builtin operators.

Previously we would get code like `x == (y = null)`, which confuses
the JS engine.  It is better to have `y = null` and `x == null` separate.

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

to

==========
dart2js cps: Disable RHS propagation in operands to builtin operators.

Previously we would get code like `x == (y = null)`, which confuses
the JS engine.  It is better to have `y = null` and `x == null` separate.

BUG=
R=sra@google.com

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

[asgerf, 2016-01-06 15:44:08]

Committed patchset #3 (id:30001) manually as
8d7c3269792f6174f90db83976aa3ffaa73c9d15 (presubmit successful).