cps-ir: Support foreign code.

cps-ir: Support foreign code.

R=kmillikin@google.com

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

Reverted: https://github.com/dart-lang/sdk/commit/40ed0daaaf71904d2da271e66ba8381ef857116d

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

[karlklose, 2015-06-12 11:01:45]

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

[karlklose, 2015-06-12 11:01:46]

Code changes in patch set 4, status file updates in patch set 5.

[Kevin Millikin (Google), 2015-06-16 11:23:10]

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2829: for (Link<ast.Node>
arguments = argumentNodes;
I'd make argumentNodes a NodeList (to avoid exposing Link) and for-in to iterate
over it.

The implementation is a little confusing.  Maybe it's clearer to rename exactly
in the body:

int count = 0;
if (minimum == null) minimum = exactly;
int maximum = exactly;
for (ast.Node in argumentNodes) {
  ++count;
  if (maximum != null && count > maximum) internalError(...);
}
if (count < minimum) internalError(...);

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2837: if (minimum != null
&& count < minimum) {
minimum should always be non-null.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2846: Link<ast.Node>
argumentNodes) {
argumentNodes is unused in this function.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2853: node.argumentsNode,
element, CallStructure.TWO_ARGS);
This uses the arguments NodeList off the Send.  It's hard to see if that's
always the same as the one that argumentNodes was derived from --- and if they
can be different it's not clear which one we should use here.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:3034: return
handleForeignSend(node, function, argumentList.nodes,
"Send" seems weird here.  handleForeignCode?

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (right):

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:344: String arguments =
node.arguments.map(access).join(', ');
Arguments are just space separated.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:346: return '(JS
${node.type} ${node.codeTemplate} $arguments $continuation)';
Arguments are enclosed in parentheses '($arguments)'.  (All variable arity parts
are like that, to make parsing simpler.)

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart (right):

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart:376:
visitForeignCode(cps_ir.ForeignCode node) {
Ixpressions get printed on a line by themselves by doing something like:

String id = names.name(node);
String arguments = ...
String continuation = ...
printStmt(id, "ForeignCode ${node.type} ${node.codeTemplate} $arguments "
    "$continuation");

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart:648: unexpectedNode(node);
addEdgeToContinuation(node.continuation);

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/js_tree_builder.dart (right):

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/js_tree_builder.dart:97:
node.dependency);
You should translate node.continuation in all cases, analogous to calls.

continueWithExpression does almost what we want, it will eventually generate
either an assignment expression statement (if the continuation parameter is
used) or just an expression statement if the continuation parameter is unused.

You might reconsider making a distinction between foreign expressions and
foreign statements --- they're all expressions and some happen to be expression
statements.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/js_tree_builder.dart:97:
node.dependency);
We discussed this offline:

1. We should make a distinction between foreign expressions and statements
somewhere (statements can't be translated to JS expressions).

2. Instead of not translating the continuation here, it would be better to just
not have a continuation in the IR in the first place.  The model would be the
IR's Throw (expression) and Rethrow.

[karlklose, 2015-06-17 11:38:46]

dd

[Kevin Millikin (Google), 2015-06-18 09:27:53]

LGTM.

[karlklose, 2015-06-18 09:38:15]

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2829: for (Link<ast.Node>
arguments = argumentNodes;
On 2015/06/16 11:23:10, Kevin Millikin (Google) wrote:
> I'd make argumentNodes a NodeList (to avoid exposing Link) and for-in to
iterate
> over it.
> 
> The implementation is a little confusing.  Maybe it's clearer to rename
exactly
> in the body:
> 
> int count = 0;
> if (minimum == null) minimum = exactly;
> int maximum = exactly;
> for (ast.Node in argumentNodes) {
>   ++count;
>   if (maximum != null && count > maximum) internalError(...);
> }
> if (count < minimum) internalError(...);

Done.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2837: if (minimum != null
&& count < minimum) {
On 2015/06/16 11:23:10, Kevin Millikin (Google) wrote:
> minimum should always be non-null.

Done.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2846: Link<ast.Node>
argumentNodes) {
On 2015/06/16 11:23:10, Kevin Millikin (Google) wrote:
> argumentNodes is unused in this function.

Done.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2853: node.argumentsNode,
element, CallStructure.TWO_ARGS);
On 2015/06/16 11:23:10, Kevin Millikin (Google) wrote:
> This uses the arguments NodeList off the Send.  It's hard to see if that's
> always the same as the one that argumentNodes was derived from --- and if they
> can be different it's not clear which one we should use here.

Done.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:3034: return
handleForeignSend(node, function, argumentList.nodes,
On 2015/06/16 11:23:10, Kevin Millikin (Google) wrote:
> "Send" seems weird here.  handleForeignCode?

Done.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart (right):

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:344: String arguments =
node.arguments.map(access).join(', ');
On 2015/06/16 11:23:10, Kevin Millikin (Google) wrote:
> Arguments are just space separated.

Done.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:346: return '(JS
${node.type} ${node.codeTemplate} $arguments $continuation)';
On 2015/06/16 11:23:10, Kevin Millikin (Google) wrote:
> Arguments are enclosed in parentheses '($arguments)'.  (All variable arity
parts
> are like that, to make parsing simpler.)

Done.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
File pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart (right):

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart:376:
visitForeignCode(cps_ir.ForeignCode node) {
On 2015/06/16 11:23:10, Kevin Millikin (Google) wrote:
> Ixpressions get printed on a line by themselves by doing something like:
> 
> String id = names.name(node);
> String arguments = ...
> String continuation = ...
> printStmt(id, "ForeignCode ${node.type} ${node.codeTemplate} $arguments "
>     "$continuation");

Done.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_tracer.dart:648: unexpectedNode(node);
On 2015/06/16 11:23:10, Kevin Millikin (Google) wrote:
> addEdgeToContinuation(node.continuation);

Done.

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js_backend/codegen/js_tree_builder.dart (right):

https://codereview.chromium.org/1185633003/diff/80001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/js_tree_builder.dart:97:
node.dependency);
On 2015/06/16 11:23:10, Kevin Millikin (Google) wrote:
> We discussed this offline:
> 
> 1. We should make a distinction between foreign expressions and statements
> somewhere (statements can't be translated to JS expressions).
> 
> 2. Instead of not translating the continuation here, it would be better to
just
> not have a continuation in the IR in the first place.  The model would be the
> IR's Throw (expression) and Rethrow.

Acknowledged.

[karlklose, 2015-06-18 11:09:55]

Committed patchset #7 (id:120001) manually as
a120ee7c9071b60edccff066dcdade078f601377 (presubmit successful).

[karlklose, 2015-06-18 13:01:51]

Patchset #8 (id:140001) has been deleted

[karlklose, 2015-06-18 13:03:12]

Patchset #8 (id:160001) has been deleted

[karlklose, 2015-06-19 07:41:47]

Patchset #8 (id:180001) has been deleted

[karlklose, 2015-06-19 07:41:55]

Patchset #8 (id:2) has been deleted

[karlklose, 2015-06-19 07:48:29]

PTAL at patch set 9.

[Kevin Millikin (Google), 2015-06-19 11:43:26]

LGTM.

[karlklose, 2015-06-19 11:45:03]

Committed patchset #10 (id:250001) manually as
3f46181244ded29edb399ad4d77b8686095cde19 (presubmit successful).