Refactor types in ssa nodes.

Refactor types in ssa nodes.

Merges in CL https://chromiumcodereview.appspot.com/10161022

Fixes issue 2564.

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

[kasperl, 2012-04-23 05:48:30]

LGTM. Maybe let Nicolas take a look too?

https://chromiumcodereview.appspot.com/10139012/diff/1001/frog/tests/leg/src/...
File frog/tests/leg/src/TypeInference4Test.dart (right):

https://chromiumcodereview.appspot.com/10139012/diff/1001/frog/tests/leg/src/...
frog/tests/leg/src/TypeInference4Test.dart:19: print(generated);
Remove printing.

https://chromiumcodereview.appspot.com/10139012/diff/1001/frog/tests/leg/src/...
frog/tests/leg/src/TypeInference4Test.dart:21:
Expect.isFalse(generated.contains('iae'));
Add comment. What are you testing for?

https://chromiumcodereview.appspot.com/10139012/diff/1001/frog/tests/leg/src/...
File frog/tests/leg/src/TypeInference5Test.dart (right):

https://chromiumcodereview.appspot.com/10139012/diff/1001/frog/tests/leg/src/...
frog/tests/leg/src/TypeInference5Test.dart:18: print(generated);
Remove printing.

https://chromiumcodereview.appspot.com/10139012/diff/1001/frog/tests/leg/src/...
frog/tests/leg/src/TypeInference5Test.dart:20:
Expect.isFalse(generated.contains('iae'));
Maybe add a comment here? What are you checking for?

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

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:695: bool isIntegerOrDouble() => this
=== INTEGER_OR_DOUBLE;
Not sure if should fix this but it is pretty weird that isIntegerOrDouble
returns false if invoked on something known to be an integer or a double. Maybe
we can come up with a better name? I know it isn't easy.

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:701: bool isWithoutSubtype() {
hasNoSubtypes?

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:725: // unknown has no influence on
the type. Always just return the other type.
unknown -> Unknown

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:729: // Avoid testing some of the
combinations by making the concreter type
concreter -> most concrete

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:735: // If we other is a super-type
return [this].
we other?

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:750: *   * int.union(num) => num.
INTEGER.union(NUMBER) => NUMBER?

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:752: *   * mutableArray.union(array)
=> array.
The casing makes it look like mutableArray is something that you can resolve (it
clearly isn't just text) but you can't find it if you look for it.

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:759: // unknown has no influence on
the type. Always just return the other type.
u -> U

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:763: // Avoid testing some of the
combinations by making the concreter type
concreter -> most concrete

[floitsch, 2012-04-23 09:21:05]

Removed isIntegerOrDouble() functions.

https://chromiumcodereview.appspot.com/10139012/diff/1001/frog/tests/leg/src/...
File frog/tests/leg/src/TypeInference4Test.dart (right):

https://chromiumcodereview.appspot.com/10139012/diff/1001/frog/tests/leg/src/...
frog/tests/leg/src/TypeInference4Test.dart:19: print(generated);
On 2012/04/23 05:48:30, kasperl wrote:
> Remove printing.

Done.

https://chromiumcodereview.appspot.com/10139012/diff/1001/frog/tests/leg/src/...
frog/tests/leg/src/TypeInference4Test.dart:21:
Expect.isFalse(generated.contains('iae'));
On 2012/04/23 05:48:30, kasperl wrote:
> Add comment. What are you testing for?

Done.

https://chromiumcodereview.appspot.com/10139012/diff/1001/frog/tests/leg/src/...
File frog/tests/leg/src/TypeInference5Test.dart (right):

https://chromiumcodereview.appspot.com/10139012/diff/1001/frog/tests/leg/src/...
frog/tests/leg/src/TypeInference5Test.dart:18: print(generated);
On 2012/04/23 05:48:30, kasperl wrote:
> Remove printing.

Done.

https://chromiumcodereview.appspot.com/10139012/diff/1001/frog/tests/leg/src/...
frog/tests/leg/src/TypeInference5Test.dart:20:
Expect.isFalse(generated.contains('iae'));
On 2012/04/23 05:48:30, kasperl wrote:
> Maybe add a comment here? What are you checking for?

Done.

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

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:695: bool isIntegerOrDouble() => this
=== INTEGER_OR_DOUBLE;
On 2012/04/23 05:48:30, kasperl wrote:
> Not sure if should fix this but it is pretty weird that isIntegerOrDouble
> returns false if invoked on something known to be an integer or a double.
Maybe
> we can come up with a better name? I know it isn't easy.

Nicolas is complaining too, but that was the reason I changed from NUMBER to
INTEGER_OR_DOUBLE. So that we could have an isNumber check that is checking for
all three number htypes.

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:701: bool isWithoutSubtype() {
On 2012/04/23 05:48:30, kasperl wrote:
> hasNoSubtypes?

Done.

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:725: // unknown has no influence on
the type. Always just return the other type.
On 2012/04/23 05:48:30, kasperl wrote:
> unknown -> Unknown

Done.

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:729: // Avoid testing some of the
combinations by making the concreter type
On 2012/04/23 05:48:30, kasperl wrote:
> concreter -> most concrete

Done.

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:735: // If we other is a super-type
return [this].
On 2012/04/23 05:48:30, kasperl wrote:
> we other? 

Done.

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:750: *   * int.union(num) => num.
On 2012/04/23 05:48:30, kasperl wrote:
> INTEGER.union(NUMBER) => NUMBER?

Done.

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:752: *   * mutableArray.union(array)
=> array.
On 2012/04/23 05:48:30, kasperl wrote:
> The casing makes it look like mutableArray is something that you can resolve
(it
> clearly isn't just text) but you can't find it if you look for it.

Changed to capital case.

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:759: // unknown has no influence on
the type. Always just return the other type.
On 2012/04/23 05:48:30, kasperl wrote:
> u -> U

Done.

https://chromiumcodereview.appspot.com/10139012/diff/1001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:763: // Avoid testing some of the
combinations by making the concreter type
On 2012/04/23 05:48:30, kasperl wrote:
> concreter -> most concrete

Done.

[ngeoffray, 2012-04-23 10:19:21]

LGTM!

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

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:670: // We need a field so that the
different static types are not canonicalized.
types -> fields

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:671: // This way we don't need to do
special work for the toString.
the toString -> [toString]

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:687: // In this context "Array" means
readable array.
I'd rather have the field named STRING_OR_READABLE_ARRAY than a comment.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:722: HType intersect(HType other) {
intersect -> intersection

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:728: // Avoid testing some of the
combinations by making the most concrete type
most concrete -> leaf type?

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:730: if (!hasNoSubtypes() &&
other.hasNoSubtypes()) {
double negation is annoying to read. You could add a hasSubtypes method.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:734: // If other is a super-type
return [this].
[other]

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:734: // If other is a super-type
return [this].
super-type -> supertype of [this]

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:762: // Avoid testing some of the
combinations by making the most concrete type
most concrete -> leaf type?

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:768: // If [other] is a super type of
[this] return [other];
super type -> supertype

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:773: // Handle the cases where
neither side is a super type of the other.
super type -> supertype

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1896: candidateType =
candidateType.union(inputType);
Please add a simple comment or example on why this is a union here.

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

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:292: HType combinedType =
value.propagatedType.intersect(node.guardedType);
Please add a simple comment or example on why this is an intersection here.

[floitsch, 2012-04-24 15:25:18]

PTAL.
I have completely refactored again.

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

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:670: // We need a field so that the
different static types are not canonicalized.
On 2012/04/23 10:19:21, ngeoffray wrote:
> types -> fields

Refactored.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:671: // This way we don't need to do
special work for the toString.
On 2012/04/23 10:19:21, ngeoffray wrote:
> the toString -> [toString]

Refactored.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:687: // In this context "Array" means
readable array.
On 2012/04/23 10:19:21, ngeoffray wrote:
> I'd rather have the field named STRING_OR_READABLE_ARRAY than a comment.

Refactored.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:722: HType intersect(HType other) {
On 2012/04/23 10:19:21, ngeoffray wrote:
> intersect -> intersection

Done.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:728: // Avoid testing some of the
combinations by making the most concrete type
On 2012/04/23 10:19:21, ngeoffray wrote:
> most concrete -> leaf type?

refactored.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:730: if (!hasNoSubtypes() &&
other.hasNoSubtypes()) {
On 2012/04/23 10:19:21, ngeoffray wrote:
> double negation is annoying to read. You could add a hasSubtypes method.

refactored.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:734: // If other is a super-type
return [this].
On 2012/04/23 10:19:21, ngeoffray wrote:
> [other]

refactored.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:734: // If other is a super-type
return [this].
On 2012/04/23 10:19:21, ngeoffray wrote:
> [other]

refactored.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:762: // Avoid testing some of the
combinations by making the most concrete type
On 2012/04/23 10:19:21, ngeoffray wrote:
> most concrete -> leaf type?

refactored.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:768: // If [other] is a super type of
[this] return [other];
On 2012/04/23 10:19:21, ngeoffray wrote:
> super type -> supertype

refactored.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:773: // Handle the cases where
neither side is a super type of the other.
On 2012/04/23 10:19:21, ngeoffray wrote:
> super type -> supertype

refactored.

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1896: candidateType =
candidateType.union(inputType);
On 2012/04/23 10:19:21, ngeoffray wrote:
> Please add a simple comment or example on why this is a union here.

Done.

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

https://chromiumcodereview.appspot.com/10139012/diff/7002/lib/compiler/implem...
lib/compiler/implementation/ssa/optimize.dart:292: HType combinedType =
value.propagatedType.intersect(node.guardedType);
On 2012/04/23 10:19:21, ngeoffray wrote:
> Please add a simple comment or example on why this is an intersection here.

Good thing you made me do this. Should have been union.

[ngeoffray, 2012-04-25 11:41:13]

LGTM!

http://codereview.chromium.org/10139012/diff/19001/frog/tests/leg/src/TypeInf...
File frog/tests/leg/src/TypeInference4Test.dart (right):

http://codereview.chromium.org/10139012/diff/19001/frog/tests/leg/src/TypeInf...
frog/tests/leg/src/TypeInference4Test.dart:20: // Don't test for illegal
argument exception. This means that the arguments
Don't test -> Test

http://codereview.chromium.org/10139012/diff/19001/frog/tests/leg/src/TypeInf...
File frog/tests/leg/src/TypeInference5Test.dart (right):

http://codereview.chromium.org/10139012/diff/19001/frog/tests/leg/src/TypeInf...
frog/tests/leg/src/TypeInference5Test.dart:19: // Don't test for illegal
argument exception. This means that the arguments
ditto

http://codereview.chromium.org/10139012/diff/19001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/nodes.dart (right):

http://codereview.chromium.org/10139012/diff/19001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/nodes.dart:730: bool isIndexablePrimitive() =>
propagatedType.isIndexable();
I think both should be named isIndexablePrimitive for consistency.

http://codereview.chromium.org/10139012/diff/19001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/nodes.dart:2044: return HType.INDEXABLE;
INDEXABLE_PRIMITIVE

http://codereview.chromium.org/10139012/diff/19001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/types.dart (right):

http://codereview.chromium.org/10139012/diff/19001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/types.dart:56: *   *
INTEGER.union(INTEGER_OR_DOUBLE) => NUMBER.
INTEGER_OR_DOUBLE -> NUMBER

http://codereview.chromium.org/10139012/diff/19001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/types.dart:57: *   * DOUBLE.union(INTEGER) =>
DOUBLE.
=> NUMBER

http://codereview.chromium.org/10139012/diff/19001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/types.dart:66: abstract HType union(HType
other);
Could you actually write unit tests for union and intersection. The code to
implement them is not trivial (eg sometimes you return this, sometimes you
return HType.FOO).

The tests should be trivial to write and will prevent us from making refactoring
bugs.

http://codereview.chromium.org/10139012/diff/19001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/types.dart:96: if (other.isBoolean() ||
other.isUnknown()) return this;
Instead of returning [this] in these methods, I would prefer if you explicitely
write it (eg HType.BOOLEAN).

http://codereview.chromium.org/10139012/diff/19001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/types.dart:148: return super.union(other);
Personal preference, but I'd prefer if you inlined the code here. Making it rely
on how the super is implemented looks brittle to me.

http://codereview.chromium.org/10139012/diff/19001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/types.dart:256: 
extra space

http://codereview.chromium.org/10139012/diff/19001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/types_propagation.dart (right):

http://codereview.chromium.org/10139012/diff/19001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/types_propagation.dart:46: if
(phi.propagatedType.isUnknown()) {
Please add a comment on why the phi may already have a type.

[floitsch, 2012-04-25 19:52:15]

https://chromiumcodereview.appspot.com/10139012/diff/19001/frog/tests/leg/src...
File frog/tests/leg/src/TypeInference4Test.dart (right):

https://chromiumcodereview.appspot.com/10139012/diff/19001/frog/tests/leg/src...
frog/tests/leg/src/TypeInference4Test.dart:20: // Don't test for illegal
argument exception. This means that the arguments
On 2012/04/25 11:41:13, ngeoffray wrote:
> Don't test -> Test

That was particularly badly worded.
done.

https://chromiumcodereview.appspot.com/10139012/diff/19001/frog/tests/leg/src...
File frog/tests/leg/src/TypeInference5Test.dart (right):

https://chromiumcodereview.appspot.com/10139012/diff/19001/frog/tests/leg/src...
frog/tests/leg/src/TypeInference5Test.dart:19: // Don't test for illegal
argument exception. This means that the arguments
On 2012/04/25 11:41:13, ngeoffray wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/10139012/diff/19001/lib/compiler/imple...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10139012/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/nodes.dart:730: bool isIndexablePrimitive() =>
propagatedType.isIndexable();
On 2012/04/25 11:41:13, ngeoffray wrote:
> I think both should be named isIndexablePrimitive for consistency.

Done.

https://chromiumcodereview.appspot.com/10139012/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/nodes.dart:2044: return HType.INDEXABLE;
On 2012/04/25 11:41:13, ngeoffray wrote:
> INDEXABLE_PRIMITIVE

Done.

https://chromiumcodereview.appspot.com/10139012/diff/19001/lib/compiler/imple...
File lib/compiler/implementation/ssa/types.dart (right):

https://chromiumcodereview.appspot.com/10139012/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/types.dart:56: *   *
INTEGER.union(INTEGER_OR_DOUBLE) => NUMBER.
On 2012/04/25 11:41:13, ngeoffray wrote:
> INTEGER_OR_DOUBLE -> NUMBER

Done.

https://chromiumcodereview.appspot.com/10139012/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/types.dart:57: *   * DOUBLE.union(INTEGER) =>
DOUBLE.
On 2012/04/25 11:41:13, ngeoffray wrote:
> => NUMBER

Done.

https://chromiumcodereview.appspot.com/10139012/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/types.dart:66: abstract HType union(HType
other);
On 2012/04/25 11:41:13, ngeoffray wrote:
> Could you actually write unit tests for union and intersection. The code to
> implement them is not trivial (eg sometimes you return this, sometimes you
> return HType.FOO).
> 
> The tests should be trivial to write and will prevent us from making
refactoring
> bugs.

Done.

https://chromiumcodereview.appspot.com/10139012/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/types.dart:96: if (other.isBoolean() ||
other.isUnknown()) return this;
On 2012/04/25 11:41:13, ngeoffray wrote:
> Instead of returning [this] in these methods, I would prefer if you
explicitely
> write it (eg HType.BOOLEAN).

Done.

https://chromiumcodereview.appspot.com/10139012/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/types.dart:148: return super.union(other);
On 2012/04/25 11:41:13, ngeoffray wrote:
> Personal preference, but I'd prefer if you inlined the code here. Making it
rely
> on how the super is implemented looks brittle to me.

Done.

https://chromiumcodereview.appspot.com/10139012/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/types.dart:256: 
On 2012/04/25 11:41:13, ngeoffray wrote:
> extra space

Done.

https://chromiumcodereview.appspot.com/10139012/diff/19001/lib/compiler/imple...
File lib/compiler/implementation/ssa/types_propagation.dart (right):

https://chromiumcodereview.appspot.com/10139012/diff/19001/lib/compiler/imple...
lib/compiler/implementation/ssa/types_propagation.dart:46: if
(phi.propagatedType.isUnknown()) {
On 2012/04/25 11:41:13, ngeoffray wrote:
> Please add a comment on why the phi may already have a type.

Done.