Properly register types on the JS foreign instruction.

Properly register types on the JS foreign instruction.
Committed: https://code.google.com/p/dart/source/detail?r=18045

[ngeoffray, 2013-01-28 12:16:37]

Going back and forth different type systems is quite painful. Before starting on
a rapid type analysis, I'll try to see how we can refactor all of this into
fewer type systems.

[polux, 2013-01-28 12:32:35]

On 2013/01/28 12:16:37, ngeoffray wrote:
> Going back and forth different type systems is quite painful. Before starting
on
> a rapid type analysis, I'll try to see how we can refactor all of this into
> fewer type systems.

If we can unify the different type representations it's great but please be
aware that at some point I'll start handling types like "Points created at line
42". Also when/if I handle abstract types, I may be maintaining a list of the
encountered concrete subtypes for each abstract type (I can surely do this in a
separate map so it's not a big deal). So the subclasses of BaseType may use more
memory than needed by other parts of the compiler.

[ahe, 2013-01-28 15:14:31]

I don't understand what direction you're heading with this CL.

You mention a refactoring. Is this a refactoring, or a bug fix?

https://codereview.chromium.org/12095011/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/12095011/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:542:
LinkBuilder<Element> types = new LinkBuilder<Element>();
This looks like a list of "elements", not "types".

https://codereview.chromium.org/12095011/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:557:
compiler.typesTask.typedNodes[jsCall] = types.toLink();
Please abstract this operation.

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

https://codereview.chromium.org/12095011/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1329: HType ssaType =
new HBoundedType.exact(type);
Why is this not a DartType?

[ngeoffray, 2013-01-28 15:34:19]

With this CL, the types of JS are propagated to the SSA backend. The native
handler, which does the resolution of the types, used to not propagate it.

This is not a refactoring.

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

https://codereview.chromium.org/12095011/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1329: HType ssaType =
new HBoundedType.exact(type);
On 2013/01/28 15:14:31, ahe wrote:
> Why is this not a DartType?

Because the SSA graph works on HType, not DartType.

[ahe, 2013-01-28 15:38:44]

Why are you making this change? Does it fix a bug, or improve generated code?

I'm asking because I don't see any tests. If this is about improving generated
code, could you provide an example?

On 2013/01/28 15:34:19, ngeoffray wrote:
> With this CL, the types of JS are propagated to the SSA backend. The native
> handler, which does the resolution of the types, used to not propagate it.
> 
> This is not a refactoring.
> 
>
https://codereview.chromium.org/12095011/diff/4001/sdk/lib/_internal/compiler...
> File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):
> 
>
https://codereview.chromium.org/12095011/diff/4001/sdk/lib/_internal/compiler...
> sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1329: HType ssaType
=
> new HBoundedType.exact(type);
> On 2013/01/28 15:14:31, ahe wrote:
> > Why is this not a DartType?
> 
> Because the SSA graph works on HType, not DartType.

I see. I think I misunderstood your first message. I thought this was a
refactoring to get less type systems.

Cheers,
Peter

[ngeoffray, 2013-01-28 15:42:14]

On 2013/01/28 15:38:44, ahe wrote:
> Why are you making this change? Does it fix a bug, or improve generated code?
> 
> I'm asking because I don't see any tests. If this is about improving generated
> code, could you provide an example?
> 
> On 2013/01/28 15:34:19, ngeoffray wrote:
> > With this CL, the types of JS are propagated to the SSA backend. The native
> > handler, which does the resolution of the types, used to not propagate it.
> > 
> > This is not a refactoring.
> > 
> >
>
https://codereview.chromium.org/12095011/diff/4001/sdk/lib/_internal/compiler...
> > File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):
> > 
> >
>
https://codereview.chromium.org/12095011/diff/4001/sdk/lib/_internal/compiler...
> > sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1329: HType
ssaType
> =
> > new HBoundedType.exact(type);
> > On 2013/01/28 15:14:31, ahe wrote:
> > > Why is this not a DartType?
> > 
> > Because the SSA graph works on HType, not DartType.
> 
> I see. I think I misunderstood your first message. I thought this was a
> refactoring to get less type systems.
> 
> Cheers,
> Peter

This is to improve generated code. Before, the return type in the following JS
expression:
JS('Window', 'window')

Was not used by the SSA backend. So it did not know that the expression had type
'Window', and could therefore not do any optimizations based on the type.

[ngeoffray, 2013-01-28 19:33:22]

On 2013/01/28 12:32:35, polux wrote:
> On 2013/01/28 12:16:37, ngeoffray wrote:
> > Going back and forth different type systems is quite painful. Before
starting
> on
> > a rapid type analysis, I'll try to see how we can refactor all of this into
> > fewer type systems.
> 
> If we can unify the different type representations it's great but please be
> aware that at some point I'll start handling types like "Points created at
line
> 42". Also when/if I handle abstract types, I may be maintaining a list of the
> encountered concrete subtypes for each abstract type (I can surely do this in
a
> separate map so it's not a big deal). So the subclasses of BaseType may use
more
> memory than needed by other parts of the compiler.

That is fine, and I concur that sophisticated type analysis needs a more
powerful type system than the traditional one. What would be great is to have
the TA type system extend the regular Dart type system, which is not the case
right now (DartType vs BaseClassType). The SSA backend should most probably work
on a high-level abstraction of the TA type system, instead of currently having
its own.

[sra1, 2013-01-28 20:12:20]

https://codereview.chromium.org/12095011/diff/4001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/native_handler.dart (right):

https://codereview.chromium.org/12095011/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/native_handler.dart:557:
compiler.typesTask.typedNodes[jsCall] = types.toLink();
Move the assignment out to the call site of ofJsCall or to the ssa builder.

NativeBehavior is just data about the operation.
I'd like to keep the NativeBehavior methods idempotent.
We will add data for use by effect analysis, global type inference, range
analysis etc to NativeBehavior.
Adding updates to other compiler phases here will rapidly get ugly as more
phases become clients of the information. 

Calling NativeBehavior.ofJsCall should not have any side-effects.
For example, the ssa builder should be able to call NativeBehavior.ofXXX rather
than rely on some other phase caching the derived information.
It will rapidly get ugly when we also need the types for global type analysis,
and the effects (when we add those.).

It would be OK to cache the NativeBehavior.

Converting from NativeBehavior to HType should be a separate operation and made
to work on NativeBehavior.NONE (returned in another branch.)
Then the same code can be used at calls to native methods to get the returned
type, which will help with the DOM benchmarks.

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

https://codereview.chromium.org/12095011/diff/4001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:1776:
world.registerInstantiatedClass(dartType.element);
This might be too broad - might be the least upper bound of other types.
The path NativeBehavior -> HType -> DartType loses precision.

This is why the NativeBehavior should be carried to HForeign.

[ngeoffray, 2013-01-30 15:16:44]

PTAL, I am now caching the NativeBehavior of JS calls, and use it in the SSA
backend. The SSA backend does not try to be clever yet on the native behavior,
it will just map whenever it can the NativeBehavior to a type it knows how to
represent.

[kasperl, 2013-02-04 09:27:50]

LGTM.

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

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1330: HType ssaType =
new HBoundedType.exact(type);
This is fairly common. Maybe have an HBoundedType constructor that takes an
element and a compiler?

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2843: print('$node
for $ssaType');
Remove printing.

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4544: ClassBaseType
classBaseType = baseType;
Unused local variable.

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1546: final HType
foreignType;
Could this be type instead of foreignType? The foreign part is sort of implied
by the fact that the field is on a foreign instruction.

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1553: : super(inputs) {
Too much indentation.

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/types.dart (right):

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:187: //
TODO(ngeoffray): Deal with Null more generally.
Null -> null. If you were going for the subtle difference between null and Null
I would write it out (something like "the type of null").

[ngeoffray, 2013-02-04 10:45:06]

Thanks Kasper.

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

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1330: HType ssaType =
new HBoundedType.exact(type);
On 2013/02/04 09:27:50, kasperl wrote:
> This is fairly common. Maybe have an HBoundedType constructor that takes an
> element and a compiler?

Good point. There are too many constructors in HBoundedType currently, I plan on
cleaning this up with named parameters, and with it add this new constructor
that takes an element.

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2843: print('$node
for $ssaType');
On 2013/02/04 09:27:50, kasperl wrote:
> Remove printing.

Done.

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:4544: ClassBaseType
classBaseType = baseType;
On 2013/02/04 09:27:50, kasperl wrote:
> Unused local variable.

Done.

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/nodes.dart (right):

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1546: final HType
foreignType;
On 2013/02/04 09:27:50, kasperl wrote:
> Could this be type instead of foreignType? The foreign part is sort of implied
> by the fact that the field is on a foreign instruction.

Done.

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/nodes.dart:1553: : super(inputs) {
On 2013/02/04 09:27:50, kasperl wrote:
> Too much indentation.

Done.

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/types.dart (right):

https://codereview.chromium.org/12095011/diff/4002/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/types.dart:187: //
TODO(ngeoffray): Deal with Null more generally.
On 2013/02/04 09:27:50, kasperl wrote:
> Null -> null. If you were going for the subtle difference between null and
Null
> I would write it out (something like "the type of null").

Done.