Added VariableUse expression to tree IR.

Added VariableUse expression to tree IR.

Variable is no longer an Expression. It must be referenced through a
VariableUse.

This makes it possible to reference a specific occurrence of a variable
in the tree, which is useful for things like finding the first or last
occurrence of a variable.

BUG=
R=kmillikin@google.com

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

[asgerf, 2015-02-25 12:40:29]

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

[asgerf, 2015-02-25 12:40:30]

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/dart_ba...
File pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart (right):

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/dart_ba...
pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart:1244:
param.writeCount--; // Not a parameter anymore.
This was technically a bug, although the reference counts are not used after
this phase, so it never manifested.

[Kevin Millikin (Google), 2015-02-26 12:43:18]

Nice.  This basically LGTM but:

I'm always a little uncomfortable when variables are treated the same on the
left-hand side of assignments/declarations and in expressions.  I'm never sure
if it's the right thing to do or not.

I'd go a step further and get rid of almost all of the functions named
visitVariable and replace them with a utility functions with a clearer name (as
far as I can see, visitVariable is always called directly, it doesn't need any
virtual dispatching, and except for the RecursiveVisitor it could as well be
something else).

What do you think?

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/dart_ba...
File pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart (right):

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/dart_ba...
pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart:835:
BuilderContext<Statement> context) {
Indentation is messed up.

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/dart_ba...
pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart:839: Expression
visitVariable(tree.Variable exp,
I'd probably get rid of this visitVariable method --- it's not required by the
superclass tree.Visitor1, and it isn't actually called for all Variables in the
input (it's not called for FunctionDefinition parameters).

Instead, I'd put a method on the BuilderContext and call it explicitly in the
two places where visitVariable is currently called.

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

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:352: js.Expression
visitVariable(tree_ir.Variable variable) {
Likewise, I'm not sure I'd call this visitVariable which suggests that I could
call it for any variable (it depends on the context, it doesn't do the right
thing for FunctionDefinition parameters).  Do we really have a js.VariableUse on
the left-hand side of an assignment?  Crazy.

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

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart:112: // Remove
assignments of form w := v from the move maps.
It looks like we remove variable's that are read and variables that are written
for slightly different reasons.  We remove the assignment in '... w ... w := v'
because adding an assignment to w before that would add one that reaches the
read of w.  And we remove the second assignment in '... w := v0 ... w := v1'
because adding an assignment to w before that would add one that doesn't reach
the use (formerly v1).

I think that would be clearer if the was something like 'removeAssignment' or
'invalidateAssignment' and it was called directly with a comment explaining why
at the two call sites.  (I'm not a fan of visitVariable :) )

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart:147: // Due to
the nop trick, the variable 'w' now has one additional write.
'The nop trick' is a bit confusing.  Isn't the extra write because in the case
where we return w and not v, we will add an assignment to w (in the caller of
copyPropagateVariable)?

Is it possible to make a setter for Assign.variable that will update the write
count when the variable is changed?

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:92: VariableUse
getMutableVariableReference(
There's probably no good reason to use Reference in the function name and Use in
the return type.  At least, I don't appreciate the distinction here.  We should
stick to one or the other.  Use is fine (it's shorter) if you like it.

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:100: class Variable extends Node
{
Is there any reason to make this a subclass of Node?  (In fact, is there any
real reason to have Node in the first place?)

[asgerf, 2015-02-27 12:05:19]

And visitVariable is gone :)

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/dart_ba...
File pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart (right):

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/dart_ba...
pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart:835:
BuilderContext<Statement> context) {
On 2015/02/26 12:43:17, kmillikin wrote:
> Indentation is messed up.

Done.

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/dart_ba...
pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart:839: Expression
visitVariable(tree.Variable exp,
On 2015/02/26 12:43:17, kmillikin wrote:
> I'd probably get rid of this visitVariable method --- it's not required by the
> superclass tree.Visitor1, and it isn't actually called for all Variables in
the
> input (it's not called for FunctionDefinition parameters).
> 
> Instead, I'd put a method on the BuilderContext and call it explicitly in the
> two places where visitVariable is currently called.

Done.

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

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/js_back...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:352: js.Expression
visitVariable(tree_ir.Variable variable) {
On 2015/02/26 12:43:18, kmillikin wrote:
> Likewise, I'm not sure I'd call this visitVariable which suggests that I could
> call it for any variable (it depends on the context, it doesn't do the right
> thing for FunctionDefinition parameters).  Do we really have a js.VariableUse
on
> the left-hand side of an assignment?  Crazy.

Renamed to buildVariableAccess.

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

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart:112: // Remove
assignments of form w := v from the move maps.
On 2015/02/26 12:43:18, kmillikin wrote:
> It looks like we remove variable's that are read and variables that are
written
> for slightly different reasons.  We remove the assignment in '... w ... w :=
v'
> because adding an assignment to w before that would add one that reaches the
> read of w.  And we remove the second assignment in '... w := v0 ... w := v1'
> because adding an assignment to w before that would add one that doesn't reach
> the use (formerly v1).
> 
> I think that would be clearer if the was something like 'removeAssignment' or
> 'invalidateAssignment' and it was called directly with a comment explaining
why
> at the two call sites.  (I'm not a fan of visitVariable :) )

Completely agree.

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart:147: // Due to
the nop trick, the variable 'w' now has one additional write.
On 2015/02/26 12:43:18, kmillikin wrote:
> 'The nop trick' is a bit confusing.  Isn't the extra write because in the case
> where we return w and not v, we will add an assignment to w (in the caller of
> copyPropagateVariable)?
> 
> Is it possible to make a setter for Assign.variable that will update the write
> count when the variable is changed?

Yeah, that's why I moved it closer to the return. I forgot to remove the
comment.

Using getters/setters to update reference counts might give a false sense of
security since it still does not decrement counts when a node gets orphaned.
Then we would need a call a destroy() method to indicate when a node has been
removed. At the end of the day, I don't think it buys us a lot. Or maybe I just
have a preference for working with "raw data" rather than "smart objects".

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart (right):

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_builder.dart:92: VariableUse
getMutableVariableReference(
On 2015/02/26 12:43:18, kmillikin wrote:
> There's probably no good reason to use Reference in the function name and Use
in
> the return type.  At least, I don't appreciate the distinction here.  We
should
> stick to one or the other.  Use is fine (it's shorter) if you like it.

Done.

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:100: class Variable extends Node
{
On 2015/02/26 12:43:18, kmillikin wrote:
> Is there any reason to make this a subclass of Node?  (In fact, is there any
> real reason to have Node in the first place?)

Currently tree_ir.Node is just used for stuff like bailout, but I think it's
fine to have it.

I was thinking of adding a method accept(Visitor v) to the node class, and then
add visitVariable and visitFunctionDefinition to the Visitor class. It's a neat
trick but in the end I decided against it since there is no direct need for it
right now.

[Kevin Millikin (Google), 2015-02-27 12:17:19]

Yay, thanks.  I think there's a small bug noted below, otherwise LGTM.

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

https://codereview.chromium.org/958603002/diff/1/pkg/compiler/lib/src/tree_ir...
pkg/compiler/lib/src/tree_ir/optimization/copy_propagator.dart:147: // Due to
the nop trick, the variable 'w' now has one additional write.
On 2015/02/27 12:05:19, asgerf wrote:
> On 2015/02/26 12:43:18, kmillikin wrote:
> > 'The nop trick' is a bit confusing.  Isn't the extra write because in the
case
> > where we return w and not v, we will add an assignment to w (in the caller
of
> > copyPropagateVariable)?
> > 
> > Is it possible to make a setter for Assign.variable that will update the
write
> > count when the variable is changed?
> 
> Yeah, that's why I moved it closer to the return. I forgot to remove the
> comment.
> 
> Using getters/setters to update reference counts might give a false sense of
> security since it still does not decrement counts when a node gets orphaned.
> Then we would need a call a destroy() method to indicate when a node has been
> removed. At the end of the day, I don't think it buys us a lot. Or maybe I
just
> have a preference for working with "raw data" rather than "smart objects".

I'm on the fence.  It's definitely a downside of trying to be too smart that you
end up with a bunch of similarly-named helpers that do things just slightly
differently (like if you ever need a different order, or to not do some of the
updates, or whatever).

I'm satisfied with leaving this explicit.

https://codereview.chromium.org/958603002/diff/20001/pkg/compiler/lib/src/dar...
File pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart (right):

https://codereview.chromium.org/958603002/diff/20001/pkg/compiler/lib/src/dar...
pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart:1250:
invalidateMovingAssignment(tree.Variable variable) {
This seems like a rename refactoring that went wrong.

[asgerf, 2015-02-27 12:20:53]

https://codereview.chromium.org/958603002/diff/20001/pkg/compiler/lib/src/dar...
File pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart (right):

https://codereview.chromium.org/958603002/diff/20001/pkg/compiler/lib/src/dar...
pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart:1250:
invalidateMovingAssignment(tree.Variable variable) {
On 2015/02/27 12:17:18, kmillikin wrote:
> This seems like a rename refactoring that went wrong.

Ah, thanks.

[asgerf, 2015-03-02 10:21:07]

Committed patchset #7 (id:110001) manually as 44125 (presubmit successful).