cps-ir: Set runtime type information for new objects that require it.

cps-ir: Set runtime type information for new objects that require it.

R=asgerf@google.com

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

[karlklose, 2015-03-18 08:43:45]

Patchset #1 (id:1) has been deleted

[karlklose, 2015-03-18 08:43:52]

Patchset #1 (id:20001) has been deleted

[karlklose, 2015-03-18 08:44:51]

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

https://codereview.chromium.org/1011403003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:363: class InvokeConstructor
extends Expression implements Invoke {
Alternatively, we could split this into two backend specific classes.

[asgerf, 2015-03-18 10:18:07]

See comment in codegen. I think the changes you made to InvokeConstructor should
be made to CreateInstance instead.

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

https://codereview.chromium.org/1011403003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:363: class InvokeConstructor
extends Expression implements Invoke {
On 2015/03/18 08:44:51, karlklose wrote:
> Alternatively, we could split this into two backend specific classes.

Following my comment in codegen, I think constructor calls should be translated
to InvokeStatic where the type arguments are passed as ordinary arguments.

https://codereview.chromium.org/1011403003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:916: 
It's not clear to me what dartType should be.

If I want to build List<String>, should I use List and String as argument, or do
I use List<String> and no arguments?

And what if I want to build Map<X, Map<Y, X>>? Do I build it recursively, like,

let Z = Map<Y, X> in Map<X, Z>

or can I build it with a single TypeExpression with dartType=Map<X, Map<Y, X>>?

In the latter case, how do I know the order of the type arguments, do I pass [Y,
X] or [X, Y]?

If it's the first case, it would be more clear if we just have a ClassElement
instead of DartType; then the arguments are just the type arguments in the order
declared in that class.

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

https://codereview.chromium.org/1011403003/diff/40001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes_sexpr.dart:358: return '(TypeExpression
${node.dartType.toString()} $args)';
Redundant toString call

https://codereview.chromium.org/1011403003/diff/40001/pkg/compiler/lib/src/da...
File pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart (right):

https://codereview.chromium.org/1011403003/diff/40001/pkg/compiler/lib/src/da...
pkg/compiler/lib/src/dart_backend/backend_ast_emitter.dart:941: throw '$node not
supported by dart backend';
Call errorUnsupportedNode

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

https://codereview.chromium.org/1011403003/diff/40001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:217: js.Expression object =
buildStaticInvoke(selector, target, arguments);
It seems we are using InvokeConstuctor to call the constructor factory, 
CreateInstance to invoke the JS constructor, and InvokeMethodDirectly to invoke
constructor bodies.

Type arguments should be set immediately after the JS constructor returns, but
here they are set after the factory returns (i.e. where the constructor is
completely done).

This will not work, because the object will not have type information while the
constructor bodies are running.

For example:

class Foo<T> {
    var field;
    Foo() {
        m();
    }
    m() { field = T; }
}

In the above case, the call to m() does not see the right value of T.

The type arguments should be passed as arguments to the constructor factory,
which should then use them in the CreateInstance instruction where the actual JS
constructor is invoked.

The code here for handling type arguments should then be in visitCreateInstance.

I think InvokeConstructor should be made Dart-specific to avoid this kind of
confusion in the future (it's not clear what kind of constructor is being
invoked). We can invoke constructor factories using InvokeStatic, passing type
arguments as ordinary arguments.

This requires updates to JsIrBuilderVisitor.buildConstructor in order to handle
the extra parameters.

[asgerf, 2015-03-19 14:53:40]

https://codereview.chromium.org/1011403003/diff/60001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/1011403003/diff/60001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:1104:
node.arguments.forEach(visitExpression);
Also visit the type information expressions.

This is probably why integrity checks fail because the integrity checker cannot
see the use of the variable, but still sees its reference count.

[karlklose, 2015-03-20 07:39:04]

https://codereview.chromium.org/1011403003/diff/60001/pkg/compiler/lib/src/tr...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/1011403003/diff/60001/pkg/compiler/lib/src/tr...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:1104:
node.arguments.forEach(visitExpression);
On 2015/03/19 14:53:40, asgerf wrote:
> Also visit the type information expressions.
> 
> This is probably why integrity checks fail because the integrity checker
cannot
> see the use of the variable, but still sees its reference count.

Thanks for catching that!

[karlklose, 2015-03-24 09:54:47]

PTAL.

I added a parameter for each type variable, if the class needs runtime type
information.

We could consider refining the tracing of RTI to track individual type variables
instead of classes, but in most actual programs there are very few classes with
more than one type variable, so it is probably not worth it.

[asgerf, 2015-03-24 12:37:22]

LGTM with a few suggestions.

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

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2463: // TODO(karlklose):
resolve effective target and type here or inline later?
I don't understand this TODO. Please clarify or resolve it.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2485: // TODO(karlklose):
optimization: share the type expression for variables.
Doesn't seem worthwhile, considering how rarely it happens, and when it does
happen, GVN will do it for us anyway.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2488: ir.Primitive value =
buildTypeExpression(variable);
It would be more clear to call buildTypeVariableAccess directly. Right now the
function looks recursive but actually isn't.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2506: }
IMO a better alternative is to implement == and hashCode in
TypeInformationParameter and then use environment.lookup.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2225: // against [Local].
That's unfortunate, but I think the comment actually makes it worse. It's too
verbose already.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2253: typeInformation =
typeArgumentParameters;
How about:

typeInformation = irParameters.sublist(firstTypeArgumentParameterIndex)

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:610: final
List<Reference<Primitive>> typeInformation;
Is there ever a need to distinguish the empty list from `null`? I think we
should stick to the empty list for cases without type information.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart (right):

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart:661: if
(node.hasTypeInformation) {
This type of check drives me crazy. Please remove hasTypeInformation and make
the list empty instead of null.

[karlklose, 2015-03-26 09:50:07]

Thanks for the review, Asger!

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

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2463: // TODO(karlklose):
resolve effective target and type here or inline later?
On 2015/03/24 12:37:21, asgerf wrote:
> I don't understand this TODO. Please clarify or resolve it.

To implement redirecting factory constructors, we can either shortcut the call
here by computing effective target and type for the call, or we can compile the
redirecting factory and try to inline the call.

I removed the TODO here, since it is only valid for one of the alternatives.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2485: // TODO(karlklose):
optimization: share the type expression for variables.
On 2015/03/24 12:37:21, asgerf wrote:
> Doesn't seem worthwhile, considering how rarely it happens, and when it does
> happen, GVN will do it for us anyway.

Done.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder.dart:2488: ir.Primitive value =
buildTypeExpression(variable);
On 2015/03/24 12:37:21, asgerf wrote:
> It would be more clear to call buildTypeVariableAccess directly. Right now the
> function looks recursive but actually isn't.

Done.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2225: // against [Local].
On 2015/03/24 12:37:21, asgerf wrote:
> That's unfortunate, but I think the comment actually makes it worse. It's too
> verbose already.

Done. But we need to address the mixup of [Element]s and [Entity]s in the IR.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2253: typeInformation =
typeArgumentParameters;
On 2015/03/24 12:37:21, asgerf wrote:
> How about:
> 
> typeInformation = irParameters.sublist(firstTypeArgumentParameterIndex)

Done.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart (right):

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:610: final
List<Reference<Primitive>> typeInformation;
Currently not. Changed.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart (right):

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/shrinking_reductions.dart:661: if
(node.hasTypeInformation) {
On 2015/03/24 12:37:22, asgerf wrote:
> This type of check drives me crazy. Please remove hasTypeInformation and make
> the list empty instead of null.

Done.

[asgerf, 2015-03-26 09:58:41]

Still LGTM.

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
File pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart (right):

https://codereview.chromium.org/1011403003/diff/140001/pkg/compiler/lib/src/c...
pkg/compiler/lib/src/cps_ir/cps_ir_builder_task.dart:2225: // against [Local].
On 2015/03/26 09:50:07, karlklose wrote:
> On 2015/03/24 12:37:21, asgerf wrote:
> > That's unfortunate, but I think the comment actually makes it worse. It's
too
> > verbose already.
> 
> Done. But we need to address the mixup of [Element]s and [Entity]s in the IR.

Completely agree.

[karlklose, 2015-03-26 10:28:01]

Committed patchset #8 (id:180001) manually as 44708 (presubmit successful).