dart2dart: Preserve variable names throughout the IR.

dart2dart: Preserve variable names throughout the IR.

I had to add register allocation to avoid unsoundness in some cases where an expression needs to reference a previous version of a variable.

Register allocation is disabled for variables without an associated element, because it will typically block inlining of these variables.

BUG=
R=kmillikin@google.com, sigurdm@google.com

Committed: https://code.google.com/p/dart/source/detail?r=37312

[asgerf, 2014-06-03 14:37:16]

https://codereview.chromium.org/312793002/diff/1/tests/language/rewrite_varia...
File tests/language/rewrite_variable_initializer_test.dart (right):

https://codereview.chromium.org/312793002/diff/1/tests/language/rewrite_varia...
tests/language/rewrite_variable_initializer_test.dart:23: return bar(x,y);
Without register allocation, this reference to 'y' will become,

z > 10 ? 20 : x

which contains an invalid reference to the old value of x.

[sigurdm, 2014-06-04 07:51:57]

lgtm

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_codegen.dart
(right):

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_codegen.dart:176:
String getVariableName(tree.Variable variable) {
Maybe move this to a getter, so the .name field is not exposed.

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart
(right):

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:87: int
index;
Maybe have a comment explaining what the index indicates.

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:423: final
Map<Element, List<Variable>> element2variables = <Element,List>{};
Is there a reason not to use <Element, List<Variable>>{} ?

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:56: /// An pure
expression that cannot throw or diverge.
An -> A

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:378: 
Extra newline

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:609: return
ref.definition.binding;
You can use
return definition.binding

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:613: Node
visitReferenceList(List<Reference> refs) {
Could be a fold

https://codereview.chromium.org/312793002/diff/1/tests/language/rewrite_varia...
File tests/language/rewrite_variable_initializer_test.dart (right):

https://codereview.chromium.org/312793002/diff/1/tests/language/rewrite_varia...
tests/language/rewrite_variable_initializer_test.dart:23: return bar(x,y);
On 2014/06/03 14:37:17, asgerf wrote:
> Without register allocation, this reference to 'y' will become,
> 
> z > 10 ? 20 : x
> 
> which contains an invalid reference to the old value of x.

This could maybe be a comment in the code for later reference.

[asgerf, 2014-06-04 09:40:51]

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_codegen.dart
(right):

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_codegen.dart:176:
String getVariableName(tree.Variable variable) {
On 2014/06/04 07:51:56, sigurdm wrote:
> Maybe move this to a getter, so the .name field is not exposed.

Not so easy since it depends on this.variableList and this.functionElement.

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart
(right):

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:87: int
index;
On 2014/06/04 07:51:56, sigurdm wrote:
> Maybe have a comment explaining what the index indicates.

It was an akward field to have in the first place, especially since it is not
enough to prevent name clashes.

I deleted the index field and introduced proper name synthesis in dart_codegen.

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:423: final
Map<Element, List<Variable>> element2variables = <Element,List>{};
On 2014/06/04 07:51:56, sigurdm wrote:
> Is there a reason not to use <Element, List<Variable>>{} ?

No, fixed it.

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:609: return
ref.definition.binding;
On 2014/06/04 07:51:56, sigurdm wrote:
> You can use
> return definition.binding

Thanks

[sigurdm, 2014-06-04 10:29:39]

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_codegen.dart
(right):

https://codereview.chromium.org/312793002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_codegen.dart:176:
String getVariableName(tree.Variable variable) {
On 2014/06/04 09:40:51, asgerf wrote:
> On 2014/06/04 07:51:56, sigurdm wrote:
> > Maybe move this to a getter, so the .name field is not exposed.
> 
> Not so easy since it depends on this.variableList and this.functionElement.

True - my mistake

[asgerf, 2014-06-04 14:14:28]

I've decided to put this on hold until we can come up with a simpler solution.

[asgerf, 2014-06-04 14:49:17]

I removed the lifting analysis and reopened the CL. A better solution is still
warranted in the future.

Example of suboptimal register allocation:

INPUT

loop3(x) {
    var n = 0;
    if (x < 100) {
        while (n < x) {
            n = n + 1;
            baz();
        }
    }
    baz();
    return n;
}

OUTPUT

loop3(x) {
  var n, n1;
  n = 0;
  L3: {
    if (x < 100) {
      L2: while (true) {
        if (n < x) {
          n1 = n + 1;
          baz();
          n = n1;
          continue L2;
        } else {
          break L3;
        }
      }
    }
  }
  baz();
  return n;
}

[Kevin Millikin (Google), 2014-06-10 11:33:01]

LGTM with comments.

https://codereview.chromium.org/312793002/diff/40001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/312793002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:577: /// Returns the
deepest of the two given scopes. The nesting order of scopes
For doc comments, the first paragraph should be a one-line description:
https://www.dartlang.org/articles/doc-comment-guidelines/

https://codereview.chromium.org/312793002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:581: Node join(Node
s1, Node s2) {
Maybe this reads better with a descriptive name.  deepest?  deepestOf?

https://codereview.chromium.org/312793002/diff/40001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:600: if (list !=
null)
Put return list on the same line as the if.  Otherwise you need braces around
the body.

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart
(right):

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:91: int
writeCount = 0;
Other than incrementing and decrementing, we do not [yet] use this, right?

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:506:
assignment = null; // ignore pure expression
=> // Ignore pure expression.
  // Kill unused argument.

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:548: if
(variable != null) { // variable is null if primitive is unused
==> // Variable ... unused.

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:549:
return new Assign(variable, definition, node.body.accept(this));
visit(node.body)

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart (right):

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:635: // If a
temporary was introduced for the initializer expression,
Hmm, the comment says "If" but the code is unconditional.  Maybe something more
like: "Record the variable name as a hint in case the initial value is named in
the output."  Or is that not quite right?

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:1194: void
useElementForVariable(ir.Primitive primitive, Element element) {
This works better as a method on Primitive, doesn't it?

[asgerf, 2014-06-10 12:22:24]

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart
(right):

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:91: int
writeCount = 0;
On 2014/06/10 11:33:01, kmillikin wrote:
> Other than incrementing and decrementing, we do not [yet] use this, right?

Right.

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:506:
assignment = null; // ignore pure expression
On 2014/06/10 11:33:01, kmillikin wrote:
> => // Ignore pure expression.
>   // Kill unused argument.

Done.

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:548: if
(variable != null) { // variable is null if primitive is unused
On 2014/06/10 11:33:01, kmillikin wrote:
> ==> // Variable ... unused.

Done.

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:549:
return new Assign(variable, definition, node.body.accept(this));
On 2014/06/10 11:33:01, kmillikin wrote:
> visit(node.body)

Done.

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart (right):

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:635: // If a
temporary was introduced for the initializer expression,
On 2014/06/10 11:33:01, kmillikin wrote:
> Hmm, the comment says "If" but the code is unconditional.  Maybe something
more
> like: "Record the variable name as a hint in case the initial value is named
in
> the output."  Or is that not quite right?

The 'if' I mention is inside the method (if element == null). For instance, if
the initializer is a parameter reference, we don't want to overwrite its
element.

But I agree that the comment is not that great. I've rewritten the comment a
bit. In particular, saying "in case," makes more sense than saying "if".

https://codereview.chromium.org/312793002/diff/80001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:1194: void
useElementForVariable(ir.Primitive primitive, Element element) {
On 2014/06/10 11:33:01, kmillikin wrote:
> This works better as a method on Primitive, doesn't it?

Yes, good idea.

[asgerf, 2014-06-12 11:49:41]

PTAL at patch 9 (delta with 8).

There was a problem when generating phi assignments, where we assign to a
variable before it is used in another assignment.

Example code:
var t = x;
x = y;
y = t;

Before patch #9, output was:
x = y;
y = x;

After patch #9, a temporary is introduced, although its name cannot currently be
recovered:
var v0 = x;
x = y;
y = v0;

[sigurdm, 2014-06-12 14:12:19]

lgtm

https://codereview.chromium.org/312793002/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart
(right):

https://codereview.chromium.org/312793002/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:552:
return;
Can be on one line

https://codereview.chromium.org/312793002/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:648:
return buildContinuationAssignment(cont.parameters[0], invoke,
You can use cont.parameters.single, it also asserts that length == 1

[asgerf, 2014-06-12 15:24:15]

https://codereview.chromium.org/312793002/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart
(right):

https://codereview.chromium.org/312793002/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:552:
return;
On 2014/06/12 14:12:19, sigurdm wrote:
> Can be on one line

Thanks.

https://codereview.chromium.org/312793002/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:648:
return buildContinuationAssignment(cont.parameters[0], invoke,
On 2014/06/12 14:12:19, sigurdm wrote:
> You can use cont.parameters.single, it also asserts that length == 1

single is only defined for Link. cont.parameters is a List.

[sigurdm, 2014-06-13 07:40:07]

https://codereview.chromium.org/312793002/diff/230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart
(right):

https://codereview.chromium.org/312793002/diff/230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/dart_backend/dart_tree.dart:648:
return buildContinuationAssignment(cont.parameters[0], invoke,
On 2014/06/12 15:24:15, asgerf wrote:
> On 2014/06/12 14:12:19, sigurdm wrote:
> > You can use cont.parameters.single, it also asserts that length == 1
> 
> single is only defined for Link. cont.parameters is a List.

No - single is defined on Iterable which List implements

[asgerf, 2014-06-13 12:51:09]

Committed patchset #16 manually as r37312 (presubmit successful).