dart2dart: Bugfix in copy propagator.

dart2dart: Bugfix in copy propagator and rename Assign.definition->value.

Do not copy propagate variable if:
- The variable is accessed from an inner function.
- We are inside a try block.
- The moving assignment is a declaration.

The last point could be fixed by making the new assignment a declaration
as well, but for now I focused on fixing bugs and not making improvements.

I suspect the copy propagator will disappear entirely after the new
register allocator lands, but these bugs are currently blocking the
integrity checker from landing, and I want to land that first.


BUG=
R=kmillikin@google.com

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

[asgerf, 2015-03-05 12:41:41]

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

[Kevin Millikin (Google), 2015-03-06 09:30:33]

Code LGTM but I have some naming suggestions and the comment at the site
mentioned below might need to be beefed up a bit.

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

https://codereview.chromium.org/980853002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart:176: if
(node.definition is VariableUse && !node.isDeclaration &&
I think it's confusing to call this field Assign.definition.  Maybe .value,
.expression, or .rightHandSide?

.isDeclaration means that it is the initial assignment to 'w', right?  It's not
immediately clear why we can't propagate that backward (is it just because the
.isDeclaration flag is not updated in that case?).  Maybe declaration isn't
quite right, because declarations are hoisted.

Finally, with this many conjuncts, I prefer to have them one per line.  It eases
(my) understanding (small ones are easy to miss, it's hard to see if all the
operators are &&, etc.) and we avoid having to reformat as much if we add or
remove one.

[asgerf, 2015-03-06 10:01:37]

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

https://codereview.chromium.org/980853002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart:176: if
(node.definition is VariableUse && !node.isDeclaration &&
On 2015/03/06 09:30:33, kmillikin wrote:
> I think it's confusing to call this field Assign.definition.  Maybe .value,
> .expression, or .rightHandSide?

It's been bothering me too. I'll rename it to .value.

> 
> .isDeclaration means that it is the initial assignment to 'w', right?  It's
not
> immediately clear why we can't propagate that backward (is it just because the
> .isDeclaration flag is not updated in that case?).  Maybe declaration isn't
> quite right, because declarations are hoisted.

isDeclaration is true when the assignment has to be a declaration and thus can't
be hoisted. This is necessary to properly scope captured variables inside a
loop.

As mentioned in the CL comment, this could be loosened, but then
copyPropagateVariable has to return a variable AND a boolean indicating if the
resulting assignment should be a declaration. I just couldn't bother right now
because I think the copy propagator is going to be overhauled or removed when
the new registor allocator lands.

Come to think of it, since mutable variables are now also used for try
statements I've taken the opportunity to update the tree IR builder so
declarations are only added for mutable variables that are actually captured.

> 
> Finally, with this many conjuncts, I prefer to have them one per line.  It
eases
> (my) understanding (small ones are easy to miss, it's hard to see if all the
> operators are &&, etc.) and we avoid having to reformat as much if we add or
> remove one.

Done.

[Kevin Millikin (Google), 2015-03-06 10:27:30]

Still LGTM.

https://codereview.chromium.org/980853002/diff/40001/pkg/compiler/lib/src/tre...
File pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart (right):

https://codereview.chromium.org/980853002/diff/40001/pkg/compiler/lib/src/tre...
pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart:175: // isolate
the uses of w to a given basic block.
Could you just update this comment to explain here why we can't move
node.isDeclaration?

[asgerf, 2015-03-06 10:49:20]

https://codereview.chromium.org/980853002/diff/40001/pkg/compiler/lib/src/tre...
File pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart (right):

https://codereview.chromium.org/980853002/diff/40001/pkg/compiler/lib/src/tre...
pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart:175: // isolate
the uses of w to a given basic block.
On 2015/03/06 10:27:30, kmillikin wrote:
> Could you just update this comment to explain here why we can't move
> node.isDeclaration?

Done.

[asgerf, 2015-03-06 10:58:58]

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