dart2js cps: Scalar replacement of aggregates

dart2js cps: Scalar replacement of aggregates

Aggregates which do not escape can be replaced with local variables.

This applies to regular objects, closures and boxes.  Closures need
their body inlined first.

Fields that are written are replaced with a MutableVariable.  Fields
that are only initialized have the reads replaced with references to
the initial value.

R=asgerf@google.com

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

[sra1, 2015-08-29 02:59:58]

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

[sra1, 2015-08-29 03:06:53]

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

[sra1, 2015-08-29 03:06:54]

Please take an initial look to say if you think this is the right direction.

The inlining in type_propagation is out for review under
https://codereview.chromium.org/1307303004/


Note that we can completely reduce this:

chain2(x) {
  f1() {
    f2() {
      f3() => x;
      return f3();
    }
    return f2();
  }
  print(f1());
}

-->
    chain2: function(x) {
      P.print(x);
    },

However, inlining (with type prop) and scalar replacement would have to be
interleaved to reduce chain1:

chain1(x) {
  f1() => x;
  f2() => f1();
  f3() => f2();
  print(f3());
}

[asgerf, 2015-08-31 09:57:30]

LGTM, I only have some superficial remarks.

The CL description has an incomplete statement ("replaced with ---").

I think we could do this during type propagation, but it would certainly add
complexity. The same situation arises with insertion of type refinement nodes
(e.g. when inlining a method call used in a condition). I'm happy with this
version until we have a better plan for inlining.

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

https://codereview.chromium.org/1319303002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/scalar_replacement.dart:125: Map<FieldElement,
MutableVariable> cells = <FieldElement, MutableVariable>{};
long line :(

https://codereview.chromium.org/1319303002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/scalar_replacement.dart:208: print('reconsider
$node');
Stray print call

https://codereview.chromium.org/1319303002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/scalar_replacement.dart:221: void visit(Node node) {
node.accept(this); }
The visit method is not necessary when extending RecursiveVisitor.

https://codereview.chromium.org/1319303002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/scalar_replacement.dart:234: /// allocations for
sclara replacement.
sclara -> scalar

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

https://codereview.chromium.org/1319303002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/task.dart:178: applyCpsPass(new
ScalarReplacer(compiler));
We should rethink the order of these passes.

ScalarReplacer and MutableVariableEliminator should be before
LoopInvariantCodeMotion and ShareInterceptors, so a call like
getInterceptor(box.x) can be shared/hoisted if the box is eliminated.

We can change the order in another CL though.

[sra1, 2015-09-02 01:12:57]

https://chromiumcodereview.appspot.com/1319303002/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/cps_ir/scalar_replacement.dart (right):

https://chromiumcodereview.appspot.com/1319303002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/scalar_replacement.dart:125: Map<FieldElement,
MutableVariable> cells = <FieldElement, MutableVariable>{};
On 2015/08/31 09:57:30, asgerf wrote:
> long line :(

Done.

https://chromiumcodereview.appspot.com/1319303002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/scalar_replacement.dart:208: print('reconsider
$node');
On 2015/08/31 09:57:30, asgerf wrote:
> Stray print call

Done.

https://chromiumcodereview.appspot.com/1319303002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/scalar_replacement.dart:221: void visit(Node node) {
node.accept(this); }
On 2015/08/31 09:57:30, asgerf wrote:
> The visit method is not necessary when extending RecursiveVisitor.

Done.

https://chromiumcodereview.appspot.com/1319303002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/cps_ir/scalar_replacement.dart:234: /// allocations for
sclara replacement.
On 2015/08/31 09:57:30, asgerf wrote:
> sclara -> scalar

Done.

https://chromiumcodereview.appspot.com/1319303002/diff/1/pkg/compiler/lib/src...
File pkg/compiler/lib/src/js_backend/codegen/task.dart (right):

https://chromiumcodereview.appspot.com/1319303002/diff/1/pkg/compiler/lib/src...
pkg/compiler/lib/src/js_backend/codegen/task.dart:178: applyCpsPass(new
ScalarReplacer(compiler));
On 2015/08/31 09:57:30, asgerf wrote:
> We should rethink the order of these passes.
> 
> ScalarReplacer and MutableVariableEliminator should be before
> LoopInvariantCodeMotion and ShareInterceptors, so a call like
> getInterceptor(box.x) can be shared/hoisted if the box is eliminated.
> 
> We can change the order in another CL though.

Acknowledged.

[sra1, 2015-09-02 05:25:17]

Committed patchset #3 (id:40001) manually as
383f8090308abe1f538d0d50dcb7d3de2447dfcb (presubmit successful).