dart2js cps: Fuse tear-off and call invocation.

dart2js cps: Fuse tear-off and call invocation.

For example:
  new Foo().bar.call(123)
    ==>
  new Foo().bar(123)

This does not trigger much right now, but it will matter when forEach
is rewritten to a loop in a later CL.

BUG=
R=floitsch@google.com

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

[asgerf, 2015-07-02 16:50:36]

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

[asgerf, 2015-07-02 16:50:36]

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

https://codereview.chromium.org/1220193003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:957:
node.arguments[startOfSequence].parent = node;
Drive-by fix, not really related.

[floitsch, 2015-07-02 18:02:41]

LGTM.

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

https://codereview.chromium.org/1220193003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:128: void changeTo(Definition<T>
newDefinition) {
Add a comment.

is this a "replaceWith" ?

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

https://codereview.chromium.org/1220193003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:497: void
replaceSubtree(Expression node, Expression replacement,
add comment about "unlink".

https://codereview.chromium.org/1220193003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:786: if (call.argumentCount !=
signature.parameterCount) return false;
Maybe add a TODO?

https://codereview.chromium.org/1220193003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:822: node.selector =
I'm not a fan of modifying the fields of a node.
Would it make sense to create a new Invoke node and replace the old one with it?

https://codereview.chromium.org/1220193003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:827: if (tearOff.hasNoUses) {
Would this happen in a different optimization by itself?

[asgerf, 2015-07-03 08:44:47]

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

https://codereview.chromium.org/1220193003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:128: void changeTo(Definition<T>
newDefinition) {
On 2015/07/02 18:02:40, floitsch wrote:
> Add a comment.
> 
> is this a "replaceWith" ?

I wanted a name that doesn't give the impression that we are doing variable
substitution (i.e. replacing ALL occurrences).

Anyways, I have removed it again since the code that used it now creates a new
reference instead.

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

https://codereview.chromium.org/1220193003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:497: void
replaceSubtree(Expression node, Expression replacement,
On 2015/07/02 18:02:41, floitsch wrote:
> add comment about "unlink".

Done.

https://codereview.chromium.org/1220193003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:786: if (call.argumentCount !=
signature.parameterCount) return false;
On 2015/07/02 18:02:40, floitsch wrote:
> Maybe add a TODO?

Done.

https://codereview.chromium.org/1220193003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:822: node.selector =
On 2015/07/02 18:02:40, floitsch wrote:
> I'm not a fan of modifying the fields of a node.
> Would it make sense to create a new Invoke node and replace the old one with
it?

Fine with me.

https://codereview.chromium.org/1220193003/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:827: if (tearOff.hasNoUses) {
On 2015/07/02 18:02:40, floitsch wrote:
> Would this happen in a different optimization by itself?

In some cases it could (but currently not implemented) but in general no,
because the getter call we eliminate has side effects in some cases.

There is a comment before one of the ifs above explaining why, but I have
elaborated on the comment down here to clear up any confusion.

[asgerf, 2015-07-03 10:42:19]

PTAL.

It turns out the very simple side-effect analysis never click because there
would always be a LetCont before the InvokeMethod. I made it look past LetConts
and LetPrims (for some primitives).

[floitsch, 2015-07-03 11:56:59]

Still LGTM.

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

https://codereview.chromium.org/1220193003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:102: bool get isSafeForReordering
=> true;
I would rather have the default be the safe "false".

https://codereview.chromium.org/1220193003/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1220193003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:765: /// postponed until
immediately before [target].
Maybe add sentence:
This predicate can be used to move expressions: an expression `e` can be moved
right before [target], if `canPostponeSideEffects(e.body, target)` is true, and
none of these expressions use `e`.

A more sophisticated analysis would track side-effect dependencies between `e`
and the expressions up to the target.

[asgerf, 2015-07-03 12:27:53]

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

https://codereview.chromium.org/1220193003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:102: bool get isSafeForReordering
=> true;
On 2015/07/03 11:56:58, floitsch wrote:
> I would rather have the default be the safe "false".

I agree, although I changed it to be abstract (no default) since now almost
every primitive needs to override it anyway.

https://codereview.chromium.org/1220193003/diff/40001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1220193003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:765: /// postponed until
immediately before [target].
On 2015/07/03 11:56:58, floitsch wrote:
> Maybe add sentence:
> This predicate can be used to move expressions: an expression `e` can be moved
> right before [target], if `canPostponeSideEffects(e.body, target)` is true,
and
> none of these expressions use `e`.
> 
> A more sophisticated analysis would track side-effect dependencies between `e`
> and the expressions up to the target.

Done.

[asgerf, 2015-07-03 12:44:52]

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