Build new IR for functions returning a constant

Build new IR for functions returning a constant.

R=ngeoffray@google.com

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

[ngeoffray, 2013-11-05 12:58:35]

Initial comments.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1535: () =>
!parseNode(compiler).hasBody());
We should make sure we don't do this.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ir/inode_builder.dart (right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inode_builder.dart:103: IFunction
result = new IFunction(nodePos(node), const<INode>[]);
Should we create that at the end of the visit, once we know the list of
statements?

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inode_builder.dart:198: IFunction
get returnFunction => returnFunctionList.last;
Are you anticipating inlining?

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inode_builder.dart:203:
List<List<IConstant>> constantsList = <List<IConstant>>[];
I think we'll just have one giant list/set of constants.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ir/inodes.dart (right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inodes.dart:4: SourceFile
sourceFile;
Probably not something we want to put on each instruction.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inodes.dart:15: static NoValue
_instance;
How about a const constructor?

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inodes.dart:37: Constant value;
final?

[lukas, 2013-11-05 17:10:49]

it's a bit hard to see the diff 1 -> 2 now because some files were renamed.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ir/inode_builder.dart (right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inode_builder.dart:103: IFunction
result = new IFunction(nodePos(node), const<INode>[]);
On 2013/11/05 12:58:35, ngeoffray wrote:
> Should we create that at the end of the visit, once we know the list of
> statements?

It's done here for now because the "IrReturn" points to the function it returns
from, so it depends on the IR.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inode_builder.dart:198: IFunction
get returnFunction => returnFunctionList.last;
On 2013/11/05 12:58:35, ngeoffray wrote:
> Are you anticipating inlining?

Yes - again, depends on the IR we use.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inode_builder.dart:203:
List<List<IConstant>> constantsList = <List<IConstant>>[];
On 2013/11/05 12:58:35, ngeoffray wrote:
> I think we'll just have one giant list/set of constants.

We have one list per function here. The reason it's a List<List> is for nested
functions (if we keep them as IrFunction nodes).

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ir/inodes.dart (right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inodes.dart:4: SourceFile
sourceFile;
On 2013/11/05 12:58:35, ngeoffray wrote:
> Probably not something we want to put on each instruction.

True!

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inodes.dart:15: static NoValue
_instance;
On 2013/11/05 12:58:35, ngeoffray wrote:
> How about a const constructor?

Yes, much better.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inodes.dart:37: Constant value;
On 2013/11/05 12:58:35, ngeoffray wrote:
> final?

Done.

[kasperl, 2013-11-06 08:30:04]

First round of comments.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:521: String message =
'The compiler crashed: ${tryToString(ex)}.\n${ex.stackTrace}';
Long line.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1531: if
(compiler.irBuilder.iNodes.containsKey(this)) {
How bad would it be to refer to the IR nodes directly from FunctionElements? If
nothing else, I'd add a couple of helpers -- one to ask if we have IR for a
FunctionElement and one to fetch the IR.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/inferrer/inferrer_visitor.dart
(right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/inferrer/inferrer_visitor.dart:590:
print(0);
I guess you're going to get rid of this before submitting.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/inferrer/inferrer_visitor.dart:591:
throw e;
rethrow.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ir/inode_builder.dart (right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inode_builder.dart:1: part of ir;
Add copyright header.

I'd make the IrBuilder live in its own library so we can have the nodes for the
IR live in a library that has (basically) no imports.

[karlklose, 2013-11-06 09:11:28]

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1534: if
(modifiers.isExternal()) return false;
You can remove this check now.

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:14: final
Map<Element, IrNode> irNodes = new Map<Element, IrNode>();
How about calling this field 'nodes'?

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:113: IrFunction
result = new IrFunction(nodeOffset(node), const<IrNode>[]);
Initialize with <IrNode>[] (growable list) and fill in the values in
l. 118 with
result.statements.addAll(context.constants)..addAll(context.statements);

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:32: List<IrNode>
statements;
The field 'statements' can be final, you only need to add elements later.

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:40: final IrFunction
function;
If the primary way of dealing with the IR is to use visitors, this information
can always be held in the context, so  you could remove it from the node.

[lukas, 2013-11-06 11:36:08]

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:521: String message =
'The compiler crashed: ${tryToString(ex)}.\n${ex.stackTrace}';
On 2013/11/06 08:30:04, kasperl wrote:
> Long line.

will be removed

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1531: if
(compiler.irBuilder.iNodes.containsKey(this)) {
On 2013/11/06 08:30:04, kasperl wrote:
> How bad would it be to refer to the IR nodes directly from FunctionElements?
If
> nothing else, I'd add a couple of helpers -- one to ask if we have IR for a
> FunctionElement and one to fetch the IR.

Done.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1535: () =>
!parseNode(compiler).hasBody());
On 2013/11/05 12:58:35, ngeoffray wrote:
> We should make sure we don't do this.

Done.

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/inferrer/inferrer_visitor.dart
(right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/inferrer/inferrer_visitor.dart:590:
print(0);
On 2013/11/06 08:30:04, kasperl wrote:
> I guess you're going to get rid of this before submitting.

yes, stuff annotated with "@lry" will be removed

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/ir/inode_builder.dart (right):

https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/ir/inode_builder.dart:1: part of ir;
On 2013/11/06 08:30:04, kasperl wrote:
> Add copyright header.
> 
> I'd make the IrBuilder live in its own library so we can have the nodes for
the
> IR live in a library that has (basically) no imports.

Done.

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1534: if
(modifiers.isExternal()) return false;
On 2013/11/06 09:11:28, karlklose wrote:
> You can remove this check now.

Done.

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:14: final
Map<Element, IrNode> irNodes = new Map<Element, IrNode>();
On 2013/11/06 09:11:28, karlklose wrote:
> How about calling this field 'nodes'?

Done.

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:113: IrFunction
result = new IrFunction(nodeOffset(node), const<IrNode>[]);
On 2013/11/06 09:11:28, karlklose wrote:
> Initialize with <IrNode>[] (growable list) and fill in the values in
> l. 118 with
> result.statements.addAll(context.constants)..addAll(context.statements);

Done.

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:32: List<IrNode>
statements;
On 2013/11/06 09:11:28, karlklose wrote:
> The field 'statements' can be final, you only need to add elements later.

Done.

https://codereview.chromium.org/57873002/diff/80001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:40: final IrFunction
function;
On 2013/11/06 09:11:28, karlklose wrote:
> If the primary way of dealing with the IR is to use visitors, this information
> can always be held in the context, so  you could remove it from the node.

OK, removed for now, we'll see how to encode returns.

[lukas, 2013-11-20 13:37:58]

Added type inference and inlining (when building SSA from ASTs) for IR nodes.

TODO: add dynamic return type checks (--enable-checked-mode) when building SSA
from IR.

[ngeoffray, 2013-11-20 14:55:26]

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:183: bool
hasIrNode(Compiler compiler);
I wouldn't put those helpers here. If someone wants to know if an element has an
IR, it should ask the IR task.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart
(right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart:28: T
returnType;
Not used.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart:42:
throw "Type infer from IR for natvie $analyzedElement";
natvie -> native

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart:76:
inferrer.addReturnTypeFor(analyzedElement, null, node.value.accept(this));
Hasn't the node been visited already?

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:54: void
unlinkTreeAndToken(Element element) {
I'd just untype element to avoid the 'as dynamic'.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:77:
this.sourceFile) : super(elements, compiler) {
Nit: One argument per line, and super on a new line.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:223: IrFunction get
currentFunction => currentFunctionList.last;
Actually, we will probably not inline at this level. And currently, the SSA
builder will do it as well.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:226: // currently
achieve with the Conctext class might overlap with LocalHandlers.
Conctext -> Context

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:239: IrConstant
enterConstant(Constant value, Node node) {
Why not a Map of Constant -> IrConstant? Like we have in the SSA.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:266: void
openContinuation() {
Unused.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:270: void
closeContinuation() {
Unused.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:13: String
sourceName;
Make those final?

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:31: int namePosition;
Make those final?

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/source_map_builder.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/source_map_builder.dart:202: Token
token;
final?

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/source_map_builder.dart:216: int
offset;
ditto

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/source_map_builder.dart:217: String
sourceName;
ditto

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1283: // TODO(lry):
handle ir functions with nested closure definitions.
Why can't you call computeClosureToClassMapping?

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:39: //
TODO(podivilov): find the right sourceFile here and remove offset checks
line too long.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:42:
node.sourcePosition = new TokenSourceFileLocation(sourceFile, beginToken);
ditto

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/from_ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/from_ir_builder.dart:1: //
Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
2012 -> 2013

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/from_ir_builder.dart:45: extends
IrNodesVisitor<HInstruction> with SsaGraphBuilderMixin {
4 space indent.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/from_ir_inliner.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/from_ir_inliner.dart:1: //
Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
2012 -> 2013

[lukas, 2013-11-20 15:13:25]

Thanks, Nicolas! Two answers below, I will include the other comments.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:183: bool
hasIrNode(Compiler compiler);
On 2013/11/20 14:55:27, ngeoffray wrote:
> I wouldn't put those helpers here. If someone wants to know if an element has
an
> IR, it should ask the IR task.

It was suggested by Kasper:
https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1283: // TODO(lry):
handle ir functions with nested closure definitions.
On 2013/11/20 14:55:27, ngeoffray wrote:
> Why can't you call computeClosureToClassMapping?

It is an AST visitor, but we don't have a tree available.

[lukas, 2013-11-21 12:20:21]

PTAL. For now I disabled the IR in --checked mode.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart
(right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart:28: T
returnType;
On 2013/11/20 14:55:27, ngeoffray wrote:
> Not used.

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart:42:
throw "Type infer from IR for natvie $analyzedElement";
On 2013/11/20 14:55:27, ngeoffray wrote:
> natvie -> native

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart:76:
inferrer.addReturnTypeFor(analyzedElement, null, node.value.accept(this));
On 2013/11/20 14:55:27, ngeoffray wrote:
> Hasn't the node been visited already?

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:54: void
unlinkTreeAndToken(Element element) {
On 2013/11/20 14:55:27, ngeoffray wrote:
> I'd just untype element to avoid the 'as dynamic'.

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:77:
this.sourceFile) : super(elements, compiler) {
On 2013/11/20 14:55:27, ngeoffray wrote:
> Nit: One argument per line, and super on a new line.

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:223: IrFunction get
currentFunction => currentFunctionList.last;
On 2013/11/20 14:55:27, ngeoffray wrote:
> Actually, we will probably not inline at this level. And currently, the SSA
> builder will do it as well.

Maybe we want to keep nested functions as such in the IR and translate to
closure classes later? That might simplify some optimizations.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:226: // currently
achieve with the Conctext class might overlap with LocalHandlers.
On 2013/11/20 14:55:27, ngeoffray wrote:
> Conctext -> Context

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:239: IrConstant
enterConstant(Constant value, Node node) {
On 2013/11/20 14:55:27, ngeoffray wrote:
> Why not a Map of Constant -> IrConstant? Like we have in the SSA.

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:266: void
openContinuation() {
On 2013/11/20 14:55:27, ngeoffray wrote:
> Unused.

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:270: void
closeContinuation() {
On 2013/11/20 14:55:27, ngeoffray wrote:
> Unused.

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:13: String
sourceName;
On 2013/11/20 14:55:27, ngeoffray wrote:
> Make those final?

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:31: int namePosition;
On 2013/11/20 14:55:27, ngeoffray wrote:
> Make those final?

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/source_map_builder.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/source_map_builder.dart:202: Token
token;
On 2013/11/20 14:55:27, ngeoffray wrote:
> final?

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/source_map_builder.dart:216: int
offset;
On 2013/11/20 14:55:27, ngeoffray wrote:
> ditto

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/source_map_builder.dart:217: String
sourceName;
On 2013/11/20 14:55:27, ngeoffray wrote:
> ditto

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:39: //
TODO(podivilov): find the right sourceFile here and remove offset checks
On 2013/11/20 14:55:27, ngeoffray wrote:
> line too long.

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:42:
node.sourcePosition = new TokenSourceFileLocation(sourceFile, beginToken);
On 2013/11/20 14:55:27, ngeoffray wrote:
> ditto

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/from_ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/from_ir_builder.dart:1: //
Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
On 2013/11/20 14:55:27, ngeoffray wrote:
> 2012 -> 2013

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/from_ir_builder.dart:45: extends
IrNodesVisitor<HInstruction> with SsaGraphBuilderMixin {
On 2013/11/20 14:55:27, ngeoffray wrote:
> 4 space indent.

Done.

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/from_ir_inliner.dart (right):

https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/from_ir_inliner.dart:1: //
Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
On 2013/11/20 14:55:27, ngeoffray wrote:
> 2012 -> 2013

Done.

[ngeoffray, 2013-11-21 15:02:04]

LGTM

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart
(right):

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart:24:
InferrerEngine<T, TypeSystem<T>> inferrer) :
Nit: ':' at the beginning of the line.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart:31:
analyzed[node] = type;
Use '=>': => analyzed[node] = type;

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/inferrer/type_graph_inferrer.dart
(right):

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/type_graph_inferrer.dart:478:
//      SimpleTypeInferrerVisitor visitor =
Remove commented code.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:3: // BSD-style
license that can be found in the LICENSE file.
Please add high-level comments on classes in this file.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:114: IrNode
buildNode(Node node) {
Unused.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:129: if
(!builder.returnOnAllBranches) {
Nit: Reverse the condition and return to avoid indentation.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:246: IrConstant
enterConstant(Constant value, Node node) {
addConstant?

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:251: IrNode
enterStatement(IrNode statement) {
addStatement?

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:11: class
PositionWithIdentifierName {
Comment.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:22: (position is
PositionWithIdentifierName) ? position.sourceName : null;
Spaces between functions/getters please.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/from_ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/from_ir_builder.dart:55:
generated[irNode] = ssaNode;
return generated[irNode] = ssaNode;

[lukas, 2013-11-21 17:14:26]

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart
(right):

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart:24:
InferrerEngine<T, TypeSystem<T>> inferrer) :
On 2013/11/21 15:02:05, ngeoffray wrote:
> Nit: ':' at the beginning of the line.

Done.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/ir_type_inferrer.dart:31:
analyzed[node] = type;
On 2013/11/21 15:02:05, ngeoffray wrote:
> Use '=>': => analyzed[node] = type;

Done.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/inferrer/type_graph_inferrer.dart
(right):

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/inferrer/type_graph_inferrer.dart:478:
//      SimpleTypeInferrerVisitor visitor =
On 2013/11/21 15:02:05, ngeoffray wrote:
> Remove commented code.

Done.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:3: // BSD-style
license that can be found in the LICENSE file.
On 2013/11/21 15:02:05, ngeoffray wrote:
> Please add high-level comments on classes in this file.

Done.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:114: IrNode
buildNode(Node node) {
On 2013/11/21 15:02:05, ngeoffray wrote:
> Unused.

Done.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:129: if
(!builder.returnOnAllBranches) {
On 2013/11/21 15:02:05, ngeoffray wrote:
> Nit: Reverse the condition and return to avoid indentation.

Done.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:246: IrConstant
enterConstant(Constant value, Node node) {
On 2013/11/21 15:02:05, ngeoffray wrote:
> addConstant?

Done.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:251: IrNode
enterStatement(IrNode statement) {
On 2013/11/21 15:02:05, ngeoffray wrote:
> addStatement?

Done.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:11: class
PositionWithIdentifierName {
On 2013/11/21 15:02:05, ngeoffray wrote:
> Comment.

Done.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:22: (position is
PositionWithIdentifierName) ? position.sourceName : null;
On 2013/11/21 15:02:05, ngeoffray wrote:
> Spaces between functions/getters please.

Done.

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ssa/from_ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/900001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ssa/from_ir_builder.dart:55:
generated[irNode] = ssaNode;
On 2013/11/21 15:02:05, ngeoffray wrote:
> return generated[irNode] = ssaNode;

Done.

[lukas, 2013-11-21 18:03:24]

+Stephen

[ngeoffray, 2013-11-22 07:33:21]

LGTM!

[karlklose, 2013-11-22 08:01:06]

DBC:

https://codereview.chromium.org/57873002/diff/970001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/970001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:86: if
(backend.isInterceptedMethod(element)) return false;
Consider storing compiler.backend in a field. (of type backend). The is test for
that field should then promote the static type to JavaSCriptBackend.

In general we should not need this special casing for the JS backend but instead
add an abstraction to Backend.

[lukas, 2013-11-22 11:51:33]

PTAL at changes in patch set 9: fix for external functions, abort IR building
early for dart backend, measure IR builder time

https://codereview.chromium.org/57873002/diff/970001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/970001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:86: if
(backend.isInterceptedMethod(element)) return false;
On 2013/11/22 08:01:06, karlklose wrote:
> Consider storing compiler.backend in a field. (of type backend). The is test
for
> that field should then promote the static type to JavaSCriptBackend.
> 
> In general we should not need this special casing for the JS backend but
instead
> add an abstraction to Backend.

Cleaned up as discussed offline. For now we abort early when not targeting
Javascript.

[ngeoffray, 2013-11-22 12:21:04]

LGTM

https://codereview.chromium.org/57873002/diff/1230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/57873002/diff/1230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1265: HGraph
graph = element.implementation.hasIrNode(compiler)
Maybe put the implementation part in the hasIrNode function.

https://codereview.chromium.org/57873002/diff/1230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/57873002/diff/1230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:22: Element
implementation = element.implementation;
In hasIrNode/irNode.

[karlklose, 2013-11-22 12:36:10]

https://codereview.chromium.org/57873002/diff/1230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/57873002/diff/1230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:183: bool
hasIrNode(Compiler compiler);
How about moving these two methods out of the element model into
compiler.irBuilder?

[lukas, 2013-11-22 16:33:03]

Move hasIr from Element to IrBuilder, and also make it handle external
functions.

https://codereview.chromium.org/57873002/diff/1230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/57873002/diff/1230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:183: bool
hasIrNode(Compiler compiler);
On 2013/11/22 12:36:11, karlklose wrote:
> How about moving these two methods out of the element model into
> compiler.irBuilder?

I initially added them on Kasper's suggestion
(https://codereview.chromium.org/57873002/diff/1/sdk/lib/_internal/compiler/im...)

But also Nicolas would prefer to have them in the IR task
(https://codereview.chromium.org/57873002/diff/640001/sdk/lib/_internal/compil...),
so I will move them there.

https://codereview.chromium.org/57873002/diff/1230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/js_backend/backend.dart (right):

https://codereview.chromium.org/57873002/diff/1230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/js_backend/backend.dart:1265: HGraph
graph = element.implementation.hasIrNode(compiler)
On 2013/11/22 12:21:04, ngeoffray wrote:
> Maybe put the implementation part in the hasIrNode function.

Done.

https://codereview.chromium.org/57873002/diff/1230001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/57873002/diff/1230001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:22: Element
implementation = element.implementation;
On 2013/11/22 12:21:04, ngeoffray wrote:
> In hasIrNode/irNode.

Done.

[lukas, 2013-11-25 09:49:38]

Committed patchset #13 manually as r30613 (presubmit successful).

[kasperl, 2013-11-25 11:32:53]

Nits:

https://codereview.chromium.org/57873002/diff/1750001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/1750001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:5: library
ir_builder;
dart2js.ir_builder?

https://codereview.chromium.org/57873002/diff/1750001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/57873002/diff/1750001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:6: // dependencies on
other parts of the system
Terminate comment with .

https://codereview.chromium.org/57873002/diff/1750001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:9: import
'../dart2jslib.dart' show Constant;
It's a little bit scary to drag in the entire Constant thing -- who knows what
kind of dependencies it has on the rest of the system. Maybe you'll have to
rework this once you start serializing/deserializing?

https://codereview.chromium.org/57873002/diff/1750001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:27: (position is
PositionWithIdentifierName) ? position.offset : position;
'is int' is simpler?

[lukas, 2013-11-25 14:50:51]

Feedback addressed in https://codereview.chromium.org/85813002/

https://codereview.chromium.org/57873002/diff/1750001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart (right):

https://codereview.chromium.org/57873002/diff/1750001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ir/ir_builder.dart:5: library
ir_builder;
On 2013/11/25 11:32:54, kasperl wrote:
> dart2js.ir_builder?

Done.

https://codereview.chromium.org/57873002/diff/1750001/sdk/lib/_internal/compi...
File sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart (right):

https://codereview.chromium.org/57873002/diff/1750001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:6: // dependencies on
other parts of the system
On 2013/11/25 11:32:54, kasperl wrote:
> Terminate comment with .

Done.

https://codereview.chromium.org/57873002/diff/1750001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:9: import
'../dart2jslib.dart' show Constant;
On 2013/11/25 11:32:54, kasperl wrote:
> It's a little bit scary to drag in the entire Constant thing -- who knows what
> kind of dependencies it has on the rest of the system. Maybe you'll have to
> rework this once you start serializing/deserializing? 

Yes - I'll address this in the next CL which will introduce serialization.

https://codereview.chromium.org/57873002/diff/1750001/sdk/lib/_internal/compi...
sdk/lib/_internal/compiler/implementation/ir/ir_nodes.dart:27: (position is
PositionWithIdentifierName) ? position.offset : position;
On 2013/11/25 11:32:54, kasperl wrote:
> 'is int' is simpler?

Done.