dart2js: improve ssa utilization of bool value types

dart2js: improve ssa utilization of bool value types

BUG=
R=sra@google.com

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

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

[Harry Terkelsen, 2015-09-29 23:48:27]

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

[Harry Terkelsen, 2015-09-30 16:30:55]

when I ran the numbers last night:

hello_world size before: 1538427
hello_world size after:  1438782

difference of 6.9%

[sra1, 2015-09-30 16:49:40]

lgtm with tweaks

https://chromiumcodereview.appspot.com/1376603003/diff/40001/pkg/compiler/lib...
File pkg/compiler/lib/src/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/1376603003/diff/40001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:209: users.forEach((HInstruction user) {
maybe just write:

for (HInstruction user in known.usedBy.toList()) {
}

https://chromiumcodereview.appspot.com/1376603003/diff/40001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:210: user.changeUse(known,
graph.addConstantBool(knownValue, compiler));
Factor this into (1) finding the HConstant (if any) (2) doing the replacement
(which can reuse the constant).

If (1) is extended (replace ints, small strings, etc) this will be cleaner.

First check usedBy is not empty (it will be on second pass) to avoid computing
the constant.

https://chromiumcodereview.appspot.com/1376603003/diff/40001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:217: replaceUsesWithConstant(node);
Reading this I ask 'why'?
Maybe call it propagateConstantValueToUses

https://chromiumcodereview.appspot.com/1376603003/diff/40001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:423: replaceUsesWithConstant(node);
Any reason not to do this first?
It might require more mock_library changes.

https://chromiumcodereview.appspot.com/1376603003/diff/40001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:833: replaceUsesWithConstant(node);
Any reason not to do this first?
We might know we are calling (const EmptyIterator()).isEmpty; which returns
true.

https://chromiumcodereview.appspot.com/1376603003/diff/40001/tests/compiler/d...
File tests/compiler/dart2js/type_inference8_test.dart (right):

https://chromiumcodereview.appspot.com/1376603003/diff/40001/tests/compiler/d...
tests/compiler/dart2js/type_inference8_test.dart:41: var falseType = new
ValueTypeMask(typesTask.boolType, new FalseConstantValue());
line length

[Harry Terkelsen, 2015-09-30 18:37:48]

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

[Harry Terkelsen, 2015-09-30 18:39:26]

https://codereview.chromium.org/1376603003/diff/40001/pkg/compiler/lib/src/ss...
File pkg/compiler/lib/src/ssa/optimize.dart (right):

https://codereview.chromium.org/1376603003/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/optimize.dart:209: users.forEach((HInstruction user) {
On 2015/09/30 16:49:39, sra1 wrote:
> maybe just write:
> 
> for (HInstruction user in known.usedBy.toList()) {
> }

Done.

https://codereview.chromium.org/1376603003/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/optimize.dart:210: user.changeUse(known,
graph.addConstantBool(knownValue, compiler));
On 2015/09/30 16:49:39, sra1 wrote:
> Factor this into (1) finding the HConstant (if any) (2) doing the replacement
> (which can reuse the constant).
> 
> If (1) is extended (replace ints, small strings, etc) this will be cleaner.
> 
> First check usedBy is not empty (it will be on second pass) to avoid computing
> the constant.

Done.

https://codereview.chromium.org/1376603003/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/optimize.dart:217: replaceUsesWithConstant(node);
On 2015/09/30 16:49:40, sra1 wrote:
> Reading this I ask 'why'?
> Maybe call it propagateConstantValueToUses

Done.

https://codereview.chromium.org/1376603003/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/optimize.dart:423: replaceUsesWithConstant(node);
On 2015/09/30 16:49:40, sra1 wrote:
> Any reason not to do this first?
> It might require more mock_library changes.

Done.

https://codereview.chromium.org/1376603003/diff/40001/pkg/compiler/lib/src/ss...
pkg/compiler/lib/src/ssa/optimize.dart:833: replaceUsesWithConstant(node);
On 2015/09/30 16:49:39, sra1 wrote:
> Any reason not to do this first?
> We might know we are calling (const EmptyIterator()).isEmpty; which returns
> true.

Done.

[Harry Terkelsen, 2015-09-30 23:54:44]

this was broken because of parameters being assigned:

foo(x) {
  x = "abc";
  print(x.length);
}

main() {
  foo(true);
}

was being rewritten to:

foo(x) {
  true = "abc";
  print(true.length);
}

main() {
  foo(true);
}

I have disabled the optimization for parameters that are the LHS of an
assignment

PTAL

[sra1, 2015-10-01 01:48:00]

lgtm

https://chromiumcodereview.appspot.com/1376603003/diff/80001/pkg/compiler/lib...
File pkg/compiler/lib/src/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/1376603003/diff/80001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:214: void
propagateConstantValueToUses(HInstruction known) {
Maybe call it 'node'.
It is not known until we test 'value'.

https://chromiumcodereview.appspot.com/1376603003/diff/80001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:219: // value since it may
Not needed.
Only parameters (and Locals) can be assigned, which is taken care of under
visitParameterValue.
The other cases are invocations which return a value and that is never 'punned'
as a location like a parameter.

https://chromiumcodereview.appspot.com/1376603003/diff/80001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:225: user.changeUse(known,
graph.addConstant(value, compiler));
You can lift this out of the loop:

HInstruction constant = graph.addConstant(value, compiler);

https://chromiumcodereview.appspot.com/1376603003/diff/80001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:233: // its uses since they are shadowed
by the assignment.
"since the uses reachable from the assignment may have values in addition to the
constant passed to the function."
(there is not really any shadowing)

The question I have is this: is the HLocalSet pattern match good enough?

foo(p) {
  for(int i = 0; i < 10; i++) {
    if (p is bool) p = 123;
  }
  return p;
}

Is the p under the 'if' guarded by a HTypeKnown?

I think we are safe since we do ssa convert many uses, just not before the first
merge or in code containing try-catch-finally.

[Harry Terkelsen, 2015-10-01 21:06:36]

https://chromiumcodereview.appspot.com/1376603003/diff/80001/pkg/compiler/lib...
File pkg/compiler/lib/src/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/1376603003/diff/80001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:214: void
propagateConstantValueToUses(HInstruction known) {
On 2015/10/01 01:48:00, sra1 wrote:
> Maybe call it 'node'.
> It is not known until we test 'value'.

Done.

https://chromiumcodereview.appspot.com/1376603003/diff/80001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:219: // value since it may
On 2015/10/01 01:48:00, sra1 wrote:
> Not needed.
> Only parameters (and Locals) can be assigned, which is taken care of under
> visitParameterValue.
> The other cases are invocations which return a value and that is never
'punned'
> as a location like a parameter.

Done.

https://chromiumcodereview.appspot.com/1376603003/diff/80001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:225: user.changeUse(known,
graph.addConstant(value, compiler));
On 2015/10/01 01:48:00, sra1 wrote:
> You can lift this out of the loop:
> 
> HInstruction constant = graph.addConstant(value, compiler);

Done.

https://chromiumcodereview.appspot.com/1376603003/diff/80001/pkg/compiler/lib...
pkg/compiler/lib/src/ssa/optimize.dart:233: // its uses since they are shadowed
by the assignment.
On 2015/10/01 01:48:00, sra1 wrote:
> "since the uses reachable from the assignment may have values in addition to
the
> constant passed to the function."
> (there is not really any shadowing)
> 
> The question I have is this: is the HLocalSet pattern match good enough?
> 
> foo(p) {
>   for(int i = 0; i < 10; i++) {
>     if (p is bool) p = 123;
>   }
>   return p;
> }
> 
> Is the p under the 'if' guarded by a HTypeKnown?
> 
> I think we are safe since we do ssa convert many uses, just not before the
first
> merge or in code containing try-catch-finally.

I just tried out this program. The p is not recognized as being the lhs of an
HLocalSet, but it seems like p is ssa converted inside the loop body:

    foo: function(p) {
      var i;
      for (p = true, i = 0; i < 10; ++i)
        if (typeof p === "boolean")
          p = 123;
      return p;
    },

[Harry Terkelsen, 2015-10-01 21:08:22]

Committed patchset #6 (id:100001) manually as
28a803d3435c6565227ad31cb5db6cbe2173ca25 (presubmit successful).