Start propagating non-primitive types in the backend, and fold instructions that know about the typ…

Start propagating non-primitive types in the backend, and fold instructions that know about the type of their receiver.
Committed: https://code.google.com/p/dart/source/detail?r=6728

[ngeoffray, 2012-04-18 14:11:48]

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/operations.dart (right):

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/operations.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
I'd like to have these Operation be our single representation of a Dart
operation and taking care of the mangling and everything. Until we're there, I'm
attaching some information to these classes to ease the transition from an SSA
instruction matching a Dart operator to a dynamic invoke.

[ahe, 2012-04-18 15:22:59]

DBC

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:752: Element
lookupMember(SourceString memberName) {
Could you put a big fat warning about this not being the right method to use for
resolving unqualified sends? In a documentation comment, please.

[floitsch, 2012-04-18 19:18:48]

LGTM.
I think we will need to differentiate between concrete and subtypes soon, but
for now this looks fine.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:388: void
registerDynamicInvocationOn(SourceString methodName,
I would prefer Element first.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:391: assert(selector !== null);
I think we need to register the invocation too.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:748: if (this === other) return this;
Why do you not look at the type?
We could even have a operator == that looks at the type.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1117: this.element = element;
I would prefer passing the element in the super call.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1183: return getter
one line.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:175: if
(node.selector.applies(parameters)) {
Even if the function doesn't apply we should set the node.element.
If it doesn't apply we should probably emit a warning.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/ssa/types.dart (right):

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/types.dart:131: // non-primitive types?
Why not. But that would require adding a isNonPrimitive check. So for now I
think this is better.

[kasperl, 2012-04-19 06:46:18]

LGTM.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:742: class HNonPrimitiveType extends
HType {
Would it make sense to have a HPrimitiveType too so you don't need the flag on
HNonPrimitiveType?

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1183: return getter
Fits on one line?

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:187: HInstruction
visitIndex(HIndex node) {
This looks like this could be refactored with a helper and used for visitIndex,
visitIndexAssign, and visitInvokeBinary.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:244: if (left.isNonPrimitive()) {
Add a comment here that explains what this does. Essentially, you're optimizing
equality checks where the left-hand side is guaranteed not to be a proper Dart
object with no user-defined equals method.

Should this check come before line 236 so we get faster null checks too?

[ngeoffray, 2012-04-19 08:09:33]

Thanks for the comments, Florian and Kasper.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/compiler.dart (right):

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:388: void
registerDynamicInvocationOn(SourceString methodName,
On 2012/04/18 19:18:48, floitsch wrote:
> I would prefer Element first.

I changed the signature to only take an element.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/compiler.dart:391: assert(selector !== null);
On 2012/04/18 19:18:48, floitsch wrote:
> I think we need to register the invocation too.

If you do that, then all 'foo' methods will be compiled. This method makes sure
only the given element is compiled.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/elements/elements.dart (right):

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/elements/elements.dart:752: Element
lookupMember(SourceString memberName) {
On 2012/04/18 15:23:00, ahe wrote:
> Could you put a big fat warning about this not being the right method to use
for
> resolving unqualified sends? In a documentation comment, please.

Done.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:742: class HNonPrimitiveType extends
HType {
On 2012/04/19 06:46:19, kasperl wrote:
> Would it make sense to have a HPrimitiveType too so you don't need the flag on
> HNonPrimitiveType?

Yes, added a TODO.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:748: if (this === other) return this;
On 2012/04/18 19:18:48, floitsch wrote:
> Why do you not look at the type?
> We could even have a operator == that looks at the type.

Well spotted. We don;t need to think about canonicalizing these HType for now,
but I wrote the code wrongly assuming they were canonicalized.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1117: this.element = element;
On 2012/04/18 19:18:48, floitsch wrote:
> I would prefer passing the element in the super call.

Done.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1183: return getter
On 2012/04/18 19:18:48, floitsch wrote:
> one line.

Done.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
File lib/compiler/implementation/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:175: if
(node.selector.applies(parameters)) {
On 2012/04/18 19:18:48, floitsch wrote:
> Even if the function doesn't apply we should set the node.element.

Why? There is no need to compile the function since we know for sure it won't be
called.

> If it doesn't apply we should probably emit a warning.

Not at this level. The warning can only be emitted by the typechecker. If it
doesn't apply, we should generate 'throw new NoSuchMethodException(...)'.

https://chromiumcodereview.appspot.com/10119010/diff/1010/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:187: HInstruction
visitIndex(HIndex node) {
On 2012/04/19 06:46:19, kasperl wrote:
> This looks like this could be refactored with a helper and used for
visitIndex,
> visitIndexAssign, and visitInvokeBinary.

Done.