dart2js cps: Use definitions by default, not references.

dart2js cps: Use definitions by default, not references.

Example of new client code:
  x.object => x.objectRef
  x.object.definition => x.object
  x.arguments[n].definition => x.argument(n)

BUG=
R=sigmund@google.com

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

[asgerf, 2016-02-29 17:17:49]

Description was changed from

==========
dart2js cps: Refactor node interfaces to focus less on references.

For example:
  x.object => x.objectRef
  x.object.definition => x.object
  x.arguments[n].definition => x.argument(n)

BUG=
==========

to

==========
dart2js cps: Use definitions by default, not references.

For example:
  x.object => x.objectRef
  x.object.definition => x.object
  x.arguments[n].definition => x.argument(n)

BUG=
==========

[asgerf, 2016-02-29 17:18:04]

Description was changed from

==========
dart2js cps: Use definitions by default, not references.

For example:
  x.object => x.objectRef
  x.object.definition => x.object
  x.arguments[n].definition => x.argument(n)

BUG=
==========

to

==========
dart2js cps: Use definitions by default, not references.

Example of new client code:
  x.object => x.objectRef
  x.object.definition => x.object
  x.arguments[n].definition => x.argument(n)

BUG=
==========

[asgerf, 2016-02-29 17:18:33]

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

[Siggi Cherem (dart-lang), 2016-02-29 18:00:00]

lgtm

https://codereview.chromium.org/1743283002/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1743283002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:2002: /// variables with
consecutive values from [argumentRefs], in the order generated
nit: reflow comment here and below  (80 col)

https://codereview.chromium.org/1743283002/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/redundant_join.dart (right):

https://codereview.chromium.org/1743283002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/redundant_join.dart:142: use.argumentRefs.add(new
Reference<Primitive>(param)..parent = use);
80 col

[asgerf, 2016-03-01 12:07:00]

https://codereview.chromium.org/1743283002/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1743283002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:2002: /// variables with
consecutive values from [argumentRefs], in the order generated
On 2016/02/29 18:00:00, Siggi Cherem (dart-lang) wrote:
> nit: reflow comment here and below  (80 col)

Actually the doc comments weren't supposed to refer to argumentRefs.

Fixed the doc comments and the long lines.

https://codereview.chromium.org/1743283002/diff/20001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/redundant_join.dart (right):

https://codereview.chromium.org/1743283002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/redundant_join.dart:142: use.argumentRefs.add(new
Reference<Primitive>(param)..parent = use);
On 2016/02/29 18:00:00, Siggi Cherem (dart-lang) wrote:
> 80 col

Done.

[asgerf, 2016-03-01 12:25:36]

Description was changed from

==========
dart2js cps: Use definitions by default, not references.

Example of new client code:
  x.object => x.objectRef
  x.object.definition => x.object
  x.arguments[n].definition => x.argument(n)

BUG=
==========

to

==========
dart2js cps: Use definitions by default, not references.

Example of new client code:
  x.object => x.objectRef
  x.object.definition => x.object
  x.arguments[n].definition => x.argument(n)

BUG=
R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/e54c4c7003477c67457367701614fdfe721a50c9
==========

[asgerf, 2016-03-01 12:25:38]

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

[sra1, 2016-03-01 17:59:52]

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

[sra1, 2016-03-01 17:59:53]

I have misgivings about this change. Perhaps there is some context I am missing.

I'm not sure the change is a overall win. Some code looks better, some worse for
the longer names.

The cost of operations is incrementally hidden. It is harder to recognize that
when a reference and its definition are used that they could be bound to locals,
or an access hoisted.

I think the extra methods will make it harder to learn the IR. The `get
arguments` case is especially subtle. I think we should live with the
representation or change the representation rather than add more 'convenience'
methods that are adding more ways to do the same thing without providing a true
abstraction.

We try to avoid abbreviations. How do you feel about changing xxxRef(s) to
xxxReference(s)?
(If we like the abbreviations, perhaps we should change the classes to Ref and
Def.)

https://codereview.chromium.org/1743283002/diff/60001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/bounds_checker.dart (right):

https://codereview.chromium.org/1743283002/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/bounds_checker.dart:499: : getValue(node.length);
It is harder to see that we could have put node.lengthRef in a local.

https://codereview.chromium.org/1743283002/diff/60001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1743283002/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:499: final Reference<Primitive>
valueRef;
valueReference

https://codereview.chromium.org/1743283002/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:549: Reference<Primitive> get
receiverRef => null;
receiverReference

https://codereview.chromium.org/1743283002/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:552: List<Reference<Primitive>>
get argumentRefs;
argumentReferences

https://codereview.chromium.org/1743283002/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:554: Iterable<Primitive> get
arguments => _dereferenceList(argumentRefs);
I find it a surprising API choice that `arguments` is not indexable.
I understand why but if I was learning this code it would be confusing.

https://codereview.chromium.org/1743283002/diff/60001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/finalize.dart (right):

https://codereview.chromium.org/1743283002/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/finalize.dart:95: Refinement refinement = new
Refinement(node.argument(0), node.type)
I'm ok with it, but this no longer implicitly asserts there is a single
argument.

https://codereview.chromium.org/1743283002/diff/60001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart (right):

https://codereview.chromium.org/1743283002/diff/60001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart:208: Primitive argument =
invoke.argument(i);
It is harder to see here that invoke.argumentRefs (originally invoke.arguments)
could possible be manually hoisted.

[sra1, 2016-03-01 17:59:57]

I have misgivings about this change. Perhaps there is some context I am missing.

I'm not sure the change is a overall win. Some code looks better, some worse for
the longer names.

The cost of operations is incrementally hidden. It is harder to recognize that
when a reference and its definition are used that they could be bound to locals,
or an access hoisted.

I think the extra methods will make it harder to learn the IR. The `get
arguments` case is especially subtle. I think we should live with the
representation or change the representation rather than add more 'convenience'
methods that are adding more ways to do the same thing without providing a true
abstraction.

We try to avoid abbreviations. How do you feel about changing xxxRef(s) to
xxxReference(s)?
(If we like the abbreviations, perhaps we should change the classes to Ref and
Def.)

[sra1, 2016-03-01 18:00:02]

I have misgivings about this change. Perhaps there is some context I am missing.

I'm not sure the change is a overall win. Some code looks better, some worse for
the longer names.

The cost of operations is incrementally hidden. It is harder to recognize that
when a reference and its definition are used that they could be bound to locals,
or an access hoisted.

I think the extra methods will make it harder to learn the IR. The `get
arguments` case is especially subtle. I think we should live with the
representation or change the representation rather than add more 'convenience'
methods that are adding more ways to do the same thing without providing a true
abstraction.

We try to avoid abbreviations. How do you feel about changing xxxRef(s) to
xxxReference(s)?
(If we like the abbreviations, perhaps we should change the classes to Ref and
Def.)

[asgerf, 2016-03-01 19:21:24]

On 2016/03/01 18:00:02, sra1 wrote:
> I have misgivings about this change. Perhaps there is some context I am
missing.
> 
> I'm not sure the change is a overall win. Some code looks better, some worse
for
> the longer names.
> 
> The cost of operations is incrementally hidden. It is harder to recognize that
> when a reference and its definition are used that they could be bound to
locals,
> or an access hoisted.

I used to feel the same way, but after using that API for about a year or so I
can't justify References being the "default" thing to access in a node.

I can't think of a time I had any reason to keep a Reference object in a local,
except when iterating over uses, and that API is the same.

> I think the extra methods will make it harder to learn the IR. The `get
> arguments` case is especially subtle. I think we should live with the
> representation or change the representation rather than add more 'convenience'
> methods that are adding more ways to do the same thing without providing a
true
> abstraction.

Really?  If it had been like this when you started looking at the CPS IR, would
this had been a problem?

I also don't like "semi-abstractions" like this, but I think it's just the
lesser
of two evils.

I do find that the code is more self-documenting now.  It shows how things are
supposed to be used.

I'm completely in favor of changing the representation if it can be done, but
I'm afraid that ship has sailed.

> We try to avoid abbreviations. How do you feel about changing xxxRef(s) to
> xxxReference(s)?
> (If we like the abbreviations, perhaps we should change the classes to Ref and
> Def.)

Once again I feel it's just the lesser of two evils.

There are 51 fields with that suffix. It's a very straightforward naming scheme.

The short name doesn't break lines as much and it doesn't steal as much
attention
from the important part of the field name.