typeInformation in CreateInstance is a kind of TypeExpression

typeInformation in CreateInstance is a kind of TypeExpression

Use TypeExpression to create the 'headless flat' type term that is used for the instance's reified type.
This is a step towards placing the type term on JSArray.

There is also some modest GVN benefits where repeated terms are shared.

R=asgerf@google.com

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

[sra1, 2016-01-26 09:10:17]

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

[asgerf, 2016-01-26 10:30:01]

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

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1760: final bool isForInstance; 
// Expression on instance has 'headless' form.
The name and comment are obscure without more details.

For a long time I thought 'headless' meant just the head of the outermost term
was omitted. For example:

List<Map<X, Y>> => [Map, X, Y]
Map<X, List<Y>> => [X, [List, Y]]

But it's really just [X, Y] in both cases.

Since dartType is unused, I think it's better to set dartType to null and
replace 'isForInstance' with a getter if it is still needed.

Also update the doc comment for the class to indicate that if dartType is
omitted, it just returns the arguments in a new array.

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

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:836: //   
typeInformation.length == classElement.typeVariables.length);
Clean up outcommented code.

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/tree_i...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (right):

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:258: }
Debug code.

I'm guessing the this was related to the missing traversal of typeInformation on
CreateInstance?

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:832: }
In this pass traversal of expressions is right-to-left, so move this above the
traversal of arguments.

Thanks for fixing it btw, the missing traversal was a bug.

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/tree_i...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:1462: node.typeInformation =
visitExpression(node.typeInformation);
I have been told we need {} around the body of multi-line if.

[sra1, 2016-01-26 22:21:08]

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

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1760: final bool isForInstance; 
// Expression on instance has 'headless' form.
On 2016/01/26 10:30:00, asgerf wrote:
> The name and comment are obscure without more details.
> 
> For a long time I thought 'headless' meant just the head of the outermost term
> was omitted. For example:
> 
> List<Map<X, Y>> => [Map, X, Y]
> Map<X, List<Y>> => [X, [List, Y]]
> 
> But it's really just [X, Y] in both cases.

It is flat because the dartType is always the 'thisType' which has a fixed list
of type parameters.

> 
> Since dartType is unused, I think it's better to set dartType to null and
> replace 'isForInstance' with a getter if it is still needed.
> 
> Also update the doc comment for the class to indicate that if dartType is
> omitted, it just returns the arguments in a new array.

I'm going to keep the dartType because it will be needed for constant folding.
Longer term I want to rewrite the type getters in terms of reading the type
information  and doing other TypeExpression operations. We will be able to
store-forward the type info and constant fold it for the new use (e.g. inlining
WhereIterable<T> -> WhereIterator<T>).

Currently we need the instance and the vector of type parameters to answer many
questions. Eventually I think the stored type should be in a form that is not
'headless', so the type operations can be performed without instances.

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

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/codegen/codegen.dart:836: //   
typeInformation.length == classElement.typeVariables.length);
On 2016/01/26 10:30:00, asgerf wrote:
> Clean up outcommented code.

Done. It had to be removed since it can be a variable after GVN

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/tree_i...
File pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart (right):

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:258: }
On 2016/01/26 10:30:00, asgerf wrote:
> Debug code.
> 
> I'm guessing the this was related to the missing traversal of typeInformation
on
> CreateInstance?

Removed.  Yes, you guessed right.

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/optimization/statement_rewriter.dart:832: }
On 2016/01/26 10:30:01, asgerf wrote:
> In this pass traversal of expressions is right-to-left, so move this above the
> traversal of arguments.
> 
> Thanks for fixing it btw, the missing traversal was a bug.

Done.

Interestingly, it makes no difference, perhaps because the whole type expression
tree is effectivelyConstant.

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/tree_i...
File pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart (right):

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/tree_i...
pkg/compiler/lib/src/tree_ir/tree_ir_nodes.dart:1462: node.typeInformation =
visitExpression(node.typeInformation);
On 2016/01/26 10:30:01, asgerf wrote:
> I have been told we need {} around the body of multi-line if.

Done.

[sra1, 2016-01-27 02:27:07]

PTAL

[sra1, 2016-01-27 02:28:01]

Description was changed from

==========
typeInformation in CreateInstance is a kind of TypeExpression

Use TypeExpression to create the 'headless' type term that is used for the
instance's reified type.
This is a step towards placing the 'headless' type term on JSArray.

There is also some modest GVN benefits where repeated terms are shared.
==========

to

==========
typeInformation in CreateInstance is a kind of TypeExpression

Use TypeExpression to create the 'headless flat' type term that is used for the
instance's reified type.
This is a step towards placing the type term on JSArray.

There is also some modest GVN benefits where repeated terms are shared.
==========

[asgerf, 2016-01-27 14:53:27]

Either commit to the flat form, or commit to the headless form, or assert in
codegen that the two forms are the same.

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

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1760: final bool isForInstance; 
// Expression on instance has 'headless' form.
CreateInstance is not always going to create the 'this' type of something.
Inlining of generic constructors will change that
(https://codereview.chromium.org/1584543002).

So the term "headless flat" bothers me, because they are not the same, and we
can't pretend they are the same forever.

I would much rather have an instruction that takes a list of values and puts
them in an immutable array.  Simple as that, a list of types, no magic
interpretation.  We could have a separate TypeVector instruction for that, or a
variant of TypeExpression could do it.

That is how the INSTANCE form seems to work right now, but the documentation
suggests something more complex is going on, and then there is this dartType
that is unused, but somehow still needed for something?

At the very least add an assertion in codegen to check that the dartType indeed
has the required form, and add a TODO explaining that the restriction is
something we want to remove eventually.  Otherwise it's going to manifest itself
as a bug later on.

(The bool field is still there btw, it should be removed).

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

https://codereview.chromium.org/1637843002/diff/20001/pkg/compiler/lib/src/cp...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1778: /// parameter vector is also
a complete form. This will allow the
Incomplete sentence at the end.

[asgerf, 2016-01-27 17:01:20]

lgtm with the assert

[sra1, 2016-01-27 19:20:03]

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

https://codereview.chromium.org/1637843002/diff/1/pkg/compiler/lib/src/cps_ir...
pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart:1760: final bool isForInstance; 
// Expression on instance has 'headless' form.
On 2016/01/27 14:53:26, asgerf wrote:
> CreateInstance is not always going to create the 'this' type of something.
> Inlining of generic constructors will change that
> (https://codereview.chromium.org/1584543002).
> 
> So the term "headless flat" bothers me, because they are not the same, and we
> can't pretend they are the same forever.

After inlining, the TypeExpression(INSTANCE) will have a different dartType to
the CreateInstance
This is OK - the arguments to TypeExpression(INSTANCE) will be changed if
necessary to TypeExpression(COMPLETE) (composition) or
ReadTypeVariable(instance, tv) (decomposition).

> 
> I would much rather have an instruction that takes a list of values and puts
> them in an immutable array.  Simple as that, a list of types, no magic
> interpretation.  We could have a separate TypeVector instruction for that, or
a
> variant of TypeExpression could do it.
> 
> That is how the INSTANCE form seems to work right now, but the documentation
> suggests something more complex is going on, and then there is this dartType
> that is unused, but somehow still needed for something?
> 
> At the very least add an assertion in codegen to check that the dartType
indeed
> has the required form, and add a TODO explaining that the restriction is
> something we want to remove eventually.  Otherwise it's going to manifest
itself
> as a bug later on.
> 
> (The bool field is still there btw, it should be removed).

Done

[sra1, 2016-01-27 19:26:44]

Description was changed from

==========
typeInformation in CreateInstance is a kind of TypeExpression

Use TypeExpression to create the 'headless flat' type term that is used for the
instance's reified type.
This is a step towards placing the type term on JSArray.

There is also some modest GVN benefits where repeated terms are shared.
==========

to

==========
typeInformation in CreateInstance is a kind of TypeExpression

Use TypeExpression to create the 'headless flat' type term that is used for the
instance's reified type.
This is a step towards placing the type term on JSArray.

There is also some modest GVN benefits where repeated terms are shared.

R=asgerf@google.com

Committed:
https://github.com/dart-lang/sdk/commit/27b7999463c781f0fe65a260da5168eb25a768db
==========

[sra1, 2016-01-27 19:26:46]

Committed patchset #3 (id:40001) manually as
27b7999463c781f0fe65a260da5168eb25a768db (presubmit successful).