dart2js cps: Clone small constants to use site.

dart2js cps: Clone small constants to use site.

BUG=
R=sra@google.com

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

[asgerf, 2015-12-16 17:20:11]

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

[sra1, 2015-12-16 18:40:12]

LGTM.

Fix the length for bools.
Add TODO for other lengths.

Is there some way to defer the decision so that it accounts for whether 'var' is
needed just for the shared constant?

https://codereview.chromium.org/1526333002/diff/40001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/1526333002/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:293: // avoid traversing the
string.
Note. The size will depend on the output encoding too (utf8 does not need
escaping, giving e.g. 3 bytes, vs 6 bytes for ascii escaped \uXXXX)

https://codereview.chromium.org/1526333002/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:296: return
'${value.primitiveValue}'.length;
We generate true and false as !0 and !1 in minified code.
We generate numbers in shorter forms too, e.g. 1230000 as 123e4 when minifying.

see js_backend/constant_emitter.dart

Given that we are about to clone the constant, does it make sense to generate
the JS for it now (for an exact measurement) and reuse the AST later?

https://codereview.chromium.org/1526333002/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:309: if
(cont.parameters[index].hint == prim.hint) {
Is this because they are both null or both something else?
I'm a bit nervous that a pass that tried to pick nice names might

https://codereview.chromium.org/1526333002/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:326: const int USE = 2; //
Minified locals usually have length 2.
Only in 'large' functions.

Can we estimate the number of temps and use 1 if the function is 'small'?
(Or at least add a TODO to refine this).

We could use more single-byte names with a little analysis to see e.g. if 'C'
and 'J' are unused.

[asgerf, 2015-12-17 15:51:53]

There is a also a change in the statement rewriter to re-enable chain
assignments that would otherwise be lost after the copying constants.

https://codereview.chromium.org/1526333002/diff/40001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/1526333002/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:293: // avoid traversing the
string.
On 2015/12/16 18:40:12, sra1 wrote:
> Note. The size will depend on the output encoding too (utf8 does not need
> escaping, giving e.g. 3 bytes, vs 6 bytes for ascii escaped \uXXXX)

Updated the comment to reflect this.

https://codereview.chromium.org/1526333002/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:296: return
'${value.primitiveValue}'.length;
On 2015/12/16 18:40:12, sra1 wrote:
> We generate true and false as !0 and !1 in minified code.
> We generate numbers in shorter forms too, e.g. 1230000 as 123e4 when
minifying.
> 
> see js_backend/constant_emitter.dart
> 
> Given that we are about to clone the constant, does it make sense to generate
> the JS for it now (for an exact measurement) and reuse the AST later?

Bools -> done.

Added a TODO for the rest. We could interface with the emitter, but I think it's
overkill.

https://codereview.chromium.org/1526333002/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:309: if
(cont.parameters[index].hint == prim.hint) {
On 2015/12/16 18:40:12, sra1 wrote:
> Is this because they are both null or both something else?
> I'm a bit nervous that a pass that tried to pick nice names might

(this is also in response to the comment below)

The register allocation will only merge variables with the same hint. But it
turns out that phi assignments in general tend to be pretty bad so I changed
this to just look any kind of phi assignment.

After merging variables, we would need def-use analysis to propagate constants
to the right uses, so our last chance to do this is after register allocation,
but before variables have actually been merged.

At that point we know which phi assignments become identity assignments, and how
many variables there are going to be.  I tried this, but I wasn't able to make
it better than this, and trying to be very clever about it actually made it
worse.  There are cases where cloning forces an "else" to be generated or a
block to get its curly braces.  It seems better to just not mess with the phis
at all, even if it's not an identity assignment.

This is just not something can solve exactly.

https://codereview.chromium.org/1526333002/diff/40001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:326: const int USE = 2; //
Minified locals usually have length 2.
On 2015/12/16 18:40:12, sra1 wrote:
> Only in 'large' functions.
> 
> Can we estimate the number of temps and use 1 if the function is 'small'?
> (Or at least add a TODO to refine this).
> 
> We could use more single-byte names with a little analysis to see e.g. if 'C'
> and 'J' are unused.

Changed to 1.  I just wanted to use the common case and I somehow got the idea
that one-byte variable names were never generated.

[sra1, 2015-12-17 17:58:28]

lgtm

[asgerf, 2015-12-18 15:31:15]

Description was changed from

==========
dart2js cps: Clone small constants to use site.

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

to

==========
dart2js cps: Clone small constants to use site.

BUG=
R=sra@google.com

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

[asgerf, 2015-12-18 15:31:16]

Committed patchset #5 (id:80001) manually as
db00be647616dd437f83d0b8c548bc611a0c37c8 (presubmit successful).