Refactor type propagation.

Refactor type propagation.

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

[kasperl, 2012-04-16 09:09:43]

LGTM! I like the new names for the types (propagated, guaranteed, likely) on the
SSA nodes.

https://chromiumcodereview.appspot.com/10098001/diff/2001/frog/tests/leg/leg....
File frog/tests/leg/leg.status (right):

https://chromiumcodereview.appspot.com/10098001/diff/2001/frog/tests/leg/leg....
frog/tests/leg/leg.status:8: TypeInference2Test: Fail # Requires liveness
analysis.
File bug?

https://chromiumcodereview.appspot.com/10098001/diff/2001/frog/tests/leg/src/...
File frog/tests/leg/src/TypeInference2Test.dart (right):

https://chromiumcodereview.appspot.com/10098001/diff/2001/frog/tests/leg/src/...
frog/tests/leg/src/TypeInference2Test.dart:17: RegExp regexp = new RegExp("i =
\\(?$anyIdentifier \\+ \\(1\\)\\)?");
Maybe add a comment that explains what you're looking for here?

https://chromiumcodereview.appspot.com/10098001/diff/2001/frog/tests/leg/src/...
File frog/tests/leg/src/TypeInferenceTest.dart (right):

https://chromiumcodereview.appspot.com/10098001/diff/2001/frog/tests/leg/src/...
frog/tests/leg/src/TypeInferenceTest.dart:53: // Some of TEST_ONE checks have
been moved into separate tests.
I'm not sure this comment adds much value.

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

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:925: // By default just copy the
guaranteed type.
Fold this comment into the /** doc */ comment?

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1657: if (propagatedType.isUnknown()
|| propagatedType.isNumber()) {
Maybe we should add more documentation to the computeDesiredTypeForInput methods
so they contain explanations on why we choose certain desired types? Whenever I
read these methods, I have to think.

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1786:
HConstant.internal(this.constant, HType type)
this.constantType in the parameter list?

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1981: if (builtin) {
Single-line return for consistency.

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/tracer.dart (right):

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/tracer.dart:169: switch
(instruction.propagatedType) {
instruction.propagedType -> type

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

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/types.dart:76: void
addDependentInstructionsToWorkList(HInstruction instruction) {
Add a comment that explains why you don't need to deal with the inputs here (and
maybe contrast that to the SpeculativeTypePropagator implementation of the same
method).

[Lasse Reichstein Nielsen, 2012-04-16 11:37:04]

dbc

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

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:901: *
This is still confusing.
The propagatedType is named from how it's derived, not what it is. Then there is
an exception where it's not derived that way, making the name entirely wrong.
Is there some way to name it for what it is? If it is anything meaningful, and
not just an intermediate value for our algorithms.

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:925: // By default just copy the
guaranteed type.
Or into a proper method body, instead of using an expression body.

[floitsch, 2012-04-16 19:59:36]

https://chromiumcodereview.appspot.com/10098001/diff/2001/frog/tests/leg/leg....
File frog/tests/leg/leg.status (right):

https://chromiumcodereview.appspot.com/10098001/diff/2001/frog/tests/leg/leg....
frog/tests/leg/leg.status:8: TypeInference2Test: Fail # Requires liveness
analysis.
On 2012/04/16 09:09:43, kasperl wrote:
> File bug?

Done.

https://chromiumcodereview.appspot.com/10098001/diff/2001/frog/tests/leg/src/...
File frog/tests/leg/src/TypeInference2Test.dart (right):

https://chromiumcodereview.appspot.com/10098001/diff/2001/frog/tests/leg/src/...
frog/tests/leg/src/TypeInference2Test.dart:17: RegExp regexp = new RegExp("i =
\\(?$anyIdentifier \\+ \\(1\\)\\)?");
On 2012/04/16 09:09:43, kasperl wrote:
> Maybe add a comment that explains what you're looking for here?

Done.

https://chromiumcodereview.appspot.com/10098001/diff/2001/frog/tests/leg/src/...
File frog/tests/leg/src/TypeInferenceTest.dart (right):

https://chromiumcodereview.appspot.com/10098001/diff/2001/frog/tests/leg/src/...
frog/tests/leg/src/TypeInferenceTest.dart:53: // Some of TEST_ONE checks have
been moved into separate tests.
On 2012/04/16 09:09:43, kasperl wrote:
> I'm not sure this comment adds much value.

removed.

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

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:901: *
On 2012/04/16 11:37:04, Lasse Reichstein Nielsen wrote:
> This is still confusing.
> The propagatedType is named from how it's derived, not what it is. Then there
is
> an exception where it's not derived that way, making the name entirely wrong.
> Is there some way to name it for what it is? If it is anything meaningful, and
> not just an intermediate value for our algorithms.

Reworded the description: The propagated type is the type that the instruction
is assumed to have. If [computeType...] and [propagatedType] differ than we have
done some optimistic type assumptions which must be guarded.

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:925: // By default just copy the
guaranteed type.
On 2012/04/16 09:09:43, kasperl wrote:
> Fold this comment into the /** doc */ comment?

Done.

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1657: if (propagatedType.isUnknown()
|| propagatedType.isNumber()) {
On 2012/04/16 09:09:43, kasperl wrote:
> Maybe we should add more documentation to the computeDesiredTypeForInput
methods
> so they contain explanations on why we choose certain desired types? Whenever
I
> read these methods, I have to think.

Added a lot of comments. I hope that's what you had in mind.

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1786:
HConstant.internal(this.constant, HType type)
On 2012/04/16 09:09:43, kasperl wrote:
> this.constantType in the parameter list?

Done.

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/nodes.dart:1981: if (builtin) {
On 2012/04/16 09:09:43, kasperl wrote:
> Single-line return for consistency.

Done.

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
File lib/compiler/implementation/ssa/tracer.dart (right):

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/tracer.dart:169: switch
(instruction.propagatedType) {
On 2012/04/16 09:09:43, kasperl wrote:
> instruction.propagedType -> type

Done.

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

https://chromiumcodereview.appspot.com/10098001/diff/2001/lib/compiler/implem...
lib/compiler/implementation/ssa/types.dart:76: void
addDependentInstructionsToWorkList(HInstruction instruction) {
On 2012/04/16 09:09:43, kasperl wrote:
> Add a comment that explains why you don't need to deal with the inputs here
(and
> maybe contrast that to the SpeculativeTypePropagator implementation of the
same
> method).

Done.