dart2js cps: Refactor CallExpressions into Primitives.

dart2js cps: Refactor CallExpressions into Primitives.

(Work-in-progress, but mostly done)

InvokeMethod and friends no longer take a continuation, but are instead
primitives bound by LetPrim.

This touches a lot of code. Some general housekeeping came along in the
process.

InvokeMethod has a "calling convention" enum on it to enable
dartReceiver and getDartArgument convenience methods.

There is a new UnsafePrimitive class to help type propagation
constant-fold InvokeMethods.  I expect the class will go away after
some changes to type propagation.

Most optimizations work exactly as before, but not all are 100%
consistent.  The consecutive push rewrite was removed entirely because
V8 does not like that.

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

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

[asgerf, 2015-11-19 18:39:14]

Description was changed from

==========
dart2js cps: Refactor CallExpressions into Primitives.

InvokeMethod and friends no longer take a continuation, but are instead
primitives bound by LetPrim.

This touches a lot of code. Some general housekeeping came along in the
process.

InvokeMethod has a "calling convention" enum on it to enable
dartReceiver and getDartArgument convenience methods.

There is a new UnsafePrimitive class to help type propagation
constant-fold InvokeMethods.  I expect the class will go away after
some changes to type propagation.

Most optimizations work exactly as before, but not all are 100%
consistent.  The consecutive push rewrite was removed entirely because
V8 does not like that.

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

to

==========
dart2js cps: Refactor CallExpressions into Primitives.

(Work-in-progress, but mostly done)

InvokeMethod and friends no longer take a continuation, but are instead
primitives bound by LetPrim.

This touches a lot of code. Some general housekeeping came along in the
process.

InvokeMethod has a "calling convention" enum on it to enable
dartReceiver and getDartArgument convenience methods.

There is a new UnsafePrimitive class to help type propagation
constant-fold InvokeMethods.  I expect the class will go away after
some changes to type propagation.

Most optimizations work exactly as before, but not all are 100%
consistent.  The consecutive push rewrite was removed entirely because
V8 does not like that.

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

[asgerf, 2015-11-19 18:39:40]

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

[sra1, 2015-11-19 21:41:46]

Looking good.

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

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_fragment.dart:129: /// The continuation body
becomes the new hole.
The comment needs rewriting

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_fragment.dart:141: /// The continuation body
becomes the new hole.
rewrite comment

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

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:266: /// Replaces this definition,
both at the binding site and at all uses sites.
Then maybe call it 'replaceWith'.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:275: void redefineAs(Primitive
newDefinition) {
assert(newDefinition.parent == null);
assert there are no uses.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:485: enum CallingConvention {
Just a thought:

We might want to make this a hand-rolled enum so the calling convention
constants can have properties.

Rather than convention==Normal, we would ask !convention.hasInterceptorPosition

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:489: Normal,
Maybe 'Direct'. 'Normal' implies abnormal but interceptors are well formed and
widespread.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:520: bool get
receiverIsIntercepted {
I'm wondering if there are any inconsistencies between receiverIsIntercepted and
checking the calling convention.
Perhaps we should delete this member.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:524: Reference<Primitive> get
dartReceiverRef {
We try to use full words where possible, e.g. dartReceiverReference

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:532: Reference<Primitive>
getDartArgumentRef(int n) {
can we drop the 'get' on these methods for symmetry with the receiver?

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:543: bool get isSafeForReordering
=> isSafeForElimination;
This feels backwards.

I'd rather this was a separate flag on UnsafePrimitive.

A function that is a static field getter is always safe for elimination but not
ordering.

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

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/insert_refinements.dart:78: /// Sets [refined] to be
the current current refinement for its value,
current current?

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/insert_refinements.dart:96: refinementFor[value] =
refined;
I think I prefer pushAction to be the last thing since that code happens after
the non-pushAction code.

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

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1599: bool isPure = element !=
null && !element.isGetter;
A bit of a diversion.

I think this line is a bit cryptic.
isPure really means is a tear-off of a method that is present because the getter
call is to a non-getter element, so that it is only possibly calling the
tear-off closure.

There could be other 'pure' functions.  I'm not sure we should do this
transformation for other cases (single use non-pure or other kinds of pure like
returning constant).

It leads to the extra code of a 'call-through' stub for foo$<n>. The
call-through stub is currently generated very late and unoptimized (i.e. get
followed by call). It would be better to inline the getter in most of these
cases.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1605: // If the getter call is
impure, we risk reordering side effects.
... unless the tear-off is immediatley prior to the closure call.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2400:
prim.isSafeForElimination = canReplace && updateValue.isConstant;
Maybe it was already safe for elimination by some other analysis and this marks
it as unsafe?

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

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:121: /// Insert a static
call to [function] at above [node].
at above -> immediately above.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/types/...
File pkg/compiler/lib/src/types/type_mask.dart (right):

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/type_mask.dart:150: //                    "class
$base.\n${classWorld.dump(base)}"));
Remember to undo this :-)

[asgerf, 2015-11-20 16:23:54]

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

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_fragment.dart:129: /// The continuation body
becomes the new hole.
On 2015/11/19 at 21:41:45, sra1 wrote:
> The comment needs rewriting

Done.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_fragment.dart:141: /// The continuation body
becomes the new hole.
On 2015/11/19 at 21:41:45, sra1 wrote:
> rewrite comment

Done.

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

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:266: /// Replaces this definition,
both at the binding site and at all uses sites.
On 2015/11/19 at 21:41:46, sra1 wrote:
> Then maybe call it 'replaceWith'.

I would like that name, but 'substituteFor' and 'replaceWith' sound too much
like they're doing the same thing (albeit with arguments reversed).

I went ahead and did the following renames:
  x.substituteFor(y) --> y.replaceUsesWith(x)
  x.redefineAs(y) --> x.replaceWith(y)

I think 'replaceUsesWith' and 'replaceWith' have a nice consistency, and it's
clear how they are different.

I did this in a separate patch, please take a look at it and see if the client
code is more/less readable than before.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:275: void redefineAs(Primitive
newDefinition) {
On 2015/11/19 at 21:41:45, sra1 wrote:
> assert(newDefinition.parent == null);
> assert there are no uses.

There are uses. They are being replaced with uses of 'newDefinition' in the
first line, and then destroy() asserts that there are no more uses.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:485: enum CallingConvention {
On 2015/11/19 at 21:41:45, sra1 wrote:
> Just a thought:
> 
> We might want to make this a hand-rolled enum so the calling convention
constants can have properties.
> 
> Rather than convention==Normal, we would ask
!convention.hasInterceptorPosition

I don't know that "interceptor position" means, but I get the idea. Let's not
overdo it, though, I think we should wait and see if we need it

FWIW, I'm planning to introduce a bitmask for all primitives, holding things
such as isSafeForElimination as well as the calling convention.
The calling convention will probably be split up into two bits: "where is the
receiver" and "are the argument shifted or not" (with the invalid combination
being unused),
corresponding to the two predicates we could put on the enum objects.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:489: Normal,
On 2015/11/19 at 21:41:45, sra1 wrote:
> Maybe 'Direct'. 'Normal' implies abnormal but interceptors are well formed and
widespread.

I would also consider NonIntercepted to be "direct" (in the sense that it calls
directly on the Dart receiver), it just has a dummy argument.

It's often hard to name the "baseline" thing, because everything else is defined
in terms of how they are different from this baseline.
So it has no defining characteristics except not being special in any way.

Normal, Plain, Ordinary, Common, Simple, Standard, pick your favourite.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:520: bool get
receiverIsIntercepted {
On 2015/11/19 at 21:41:45, sra1 wrote:
> I'm wondering if there are any inconsistencies between receiverIsIntercepted
and checking the calling convention.
> Perhaps we should delete this member.

Done.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:524: Reference<Primitive> get
dartReceiverRef {
On 2015/11/19 at 21:41:46, sra1 wrote:
> We try to use full words where possible, e.g. dartReceiverReference

Done.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:532: Reference<Primitive>
getDartArgumentRef(int n) {
On 2015/11/19 at 21:41:45, sra1 wrote:
> can we drop the 'get' on these methods for symmetry with the receiver?

Done.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:543: bool get isSafeForReordering
=> isSafeForElimination;
On 2015/11/19 at 21:41:46, sra1 wrote:
> This feels backwards.
> 
> I'd rather this was a separate flag on UnsafePrimitive.
> 
> A function that is a static field getter is always safe for elimination but
not ordering.

Done.

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

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/insert_refinements.dart:78: /// Sets [refined] to be
the current current refinement for its value,
On 2015/11/19 at 21:41:46, sra1 wrote:
> current current?

It is even more current than usual

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/insert_refinements.dart:96: refinementFor[value] =
refined;
On 2015/11/19 at 21:41:46, sra1 wrote:
> I think I prefer pushAction to be the last thing since that code happens after
the non-pushAction code.

Done.

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

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1599: bool isPure = element !=
null && !element.isGetter;
On 2015/11/19 21:41:46, sra1 wrote:
> A bit of a diversion.
> 
> I think this line is a bit cryptic.
> isPure really means is a tear-off of a method that is present because the
getter
> call is to a non-getter element, so that it is only possibly calling the
> tear-off closure.
> 
> There could be other 'pure' functions.  I'm not sure we should do this
> transformation for other cases (single use non-pure or other kinds of pure
like
> returning constant).
> 
> It leads to the extra code of a 'call-through' stub for foo$<n>. The
> call-through stub is currently generated very late and unoptimized (i.e. get
> followed by call). It would be better to inline the getter in most of these
> cases.

I expanded with a comment explaining why we do this.  The safety criterion
really is whether the getter call is pure, so I prefer to keep the variable
name.

Separate from the safety check, we should also evaluate the potential benefit of
the transformation. I added another check to bail out if this is definitely not
a tear-off.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:1605: // If the getter call is
impure, we risk reordering side effects.
On 2015/11/19 at 21:41:46, sra1 wrote:
> ... unless the tear-off is immediatley prior to the closure call.

Done.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:2400:
prim.isSafeForElimination = canReplace && updateValue.isConstant;
On 2015/11/19 at 21:41:46, sra1 wrote:
> Maybe it was already safe for elimination by some other analysis and this
marks it as unsafe?

We have to clear the flag, since 'setResult' may be called with a constant
initially, and then, later in the fixed-point iteration, called with a less
precise (non-constant) value.

This means we lose the information from the other analysis, but for the time
being there is no other analysis.

Note that this method only accepts UnsafePrimitives so it won't accidentally
mark Interceptor, TypeTest, etc as unsafe. This is only to allow
constant-folding of method invocations and other things normally are unsafe.

It's not pretty, but it's one of the things I left this way because I wanted the
CL to preserve existing behavior.  I want to change how constant folding works,
and this is one of the reasons. It's awkward.  But that's for a another CL,
hopefully very soon.

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

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/unsugar.dart:121: /// Insert a static
call to [function] at above [node].
On 2015/11/19 at 21:41:46, sra1 wrote:
> at above -> immediately above.

Done.

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/types/...
File pkg/compiler/lib/src/types/type_mask.dart (right):

https://codereview.chromium.org/1458703007/diff/1/pkg/compiler/lib/src/types/...
pkg/compiler/lib/src/types/type_mask.dart:150: //                    "class
$base.\n${classWorld.dump(base)}"));
On 2015/11/19 21:41:46, sra1 wrote:
> Remember to undo this :-)

Done.

[sra1, 2015-11-20 23:47:23]

lgtm

looks great!

[asgerf, 2015-11-23 09:25:44]

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

[Kevin Millikin (Google), 2015-11-23 10:22:44]

LGTM (looks great to me).

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/cps_fragment.dart (right):

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_fragment.dart:132: sourceInformation));
I'm not sure what the indentation is intended to be here.  It seems inconsistent
with the function just below.

I suggest just indenting 4.  Alternately, you could line up with the call's
first argument (like the parameters in the declaration).

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_fragment.dart:264: Primitive thisArgument,
It seems to me that thisArgument should come before the rest of the arguments. 
That matches the order of the FunctionDefinition and InvokeMethod constructors.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_fragment.dart:272: Continuation returnCont =
target.returnContinuation;
Strange that this is still a continuation but calls don't take a continuation. 
TODO when we get rid of continuations entirely: start using a name something
like label for join points and a function's normal exit.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2599: receiver, target,
selector, arguments, sourceInformation));
Indentation is strange in some places where a function expression was removed. 
Like here.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2644: return addPrimitive(new
ir.InvokeConstructor(
And here.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/insert_refinements.dart (right):

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/insert_refinements.dart:118: // Do not try to refine
the receiver of closure calls; the class world
Let's negate the condition of the if, get rid of the else, and move this comment
just before the if.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/insert_refinements.dart:231: // Do not push the
branch continuations here. visitBranch will do that.
Let's negate the condition, swap then and else, eliminate the empty else, and
move this comment to just before the if.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:764: bool
usedToBeCallExpression(Primitive prim) {
Ha ha.  Are these exactly the UnsafePrimitives?

[asgerf, 2015-11-23 10:48:57]

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/cps_fragment.dart (right):

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_fragment.dart:132: sourceInformation));
On 2015/11/23 10:22:44, Kevin Millikin (Google) wrote:
> I'm not sure what the indentation is intended to be here.  It seems
inconsistent
> with the function just below.
> 
> I suggest just indenting 4.  Alternately, you could line up with the call's
> first argument (like the parameters in the declaration).

Done.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_fragment.dart:264: Primitive thisArgument,
On 2015/11/23 10:22:43, Kevin Millikin (Google) wrote:
> It seems to me that thisArgument should come before the rest of the arguments.

> That matches the order of the FunctionDefinition and InvokeMethod
constructors.

Done.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_fragment.dart:272: Continuation returnCont =
target.returnContinuation;
On 2015/11/23 10:22:44, Kevin Millikin (Google) wrote:
> Strange that this is still a continuation but calls don't take a continuation.

> TODO when we get rid of continuations entirely: start using a name something
> like label for join points and a function's normal exit.

Acknowledged.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart (right):

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2599: receiver, target,
selector, arguments, sourceInformation));
On 2015/11/23 10:22:44, Kevin Millikin (Google) wrote:
> Indentation is strange in some places where a function expression was removed.

> Like here.

Done.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2644: return addPrimitive(new
ir.InvokeConstructor(
On 2015/11/23 10:22:44, Kevin Millikin (Google) wrote:
> And here.

Done.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/insert_refinements.dart (right):

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/insert_refinements.dart:118: // Do not try to refine
the receiver of closure calls; the class world
On 2015/11/23 10:22:44, Kevin Millikin (Google) wrote:
> Let's negate the condition of the if, get rid of the else, and move this
comment
> just before the if.

Done.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/insert_refinements.dart:231: // Do not push the
branch continuations here. visitBranch will do that.
On 2015/11/23 10:22:44, Kevin Millikin (Google) wrote:
> Let's negate the condition, swap then and else, eliminate the empty else, and
> move this comment to just before the if.

Done.

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/type_propagation.dart (right):

https://codereview.chromium.org/1458703007/diff/220001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/type_propagation.dart:764: bool
usedToBeCallExpression(Primitive prim) {
On 2015/11/23 10:22:44, Kevin Millikin (Google) wrote:
> Ha ha.  Are these exactly the UnsafePrimitives?

It was missing Yield and Await, but otherwise yes.

Changed to 'is UnsafePrimitive'.

[asgerf, 2015-11-23 11:29:49]

Committed patchset #16 (id:280001) manually as
cb8d7994cee3771b95d03aadd2b6c16398e5da97 (presubmit successful).