Support break and labeled statements.

Support break and labeled statements.

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

[Lasse Reichstein Nielsen, 2012-02-20 13:56:15]

https://chromiumcodereview.appspot.com/9421035/diff/1/frog/leg/ssa/nodes.dart
File frog/leg/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/1/frog/leg/ssa/nodes.dart...
frog/leg/ssa/nodes.dart:354: HBlockInformation blockInformation = null;
I store both loop information and labeled block information here, depending on
what the block represents. Accessors below check the type before returning it.

https://chromiumcodereview.appspot.com/9421035/diff/1/frog/leg/typechecker.dart
File frog/leg/typechecker.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/1/frog/leg/typechecker.da...
frog/leg/typechecker.dart:590: return StatementType.NOT_RETURNING;
Is this correct? I see visitThrow returns RETURNING, but would that change if we
had catch?

[floitsch, 2012-02-20 19:01:54]

small things, but almost LGTM.

https://chromiumcodereview.appspot.com/9421035/diff/1/frog/leg/ssa/nodes.dart
File frog/leg/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/1/frog/leg/ssa/nodes.dart...
frog/leg/ssa/nodes.dart:354: HBlockInformation blockInformation = null;
On 2012/02/20 13:56:16, Lasse Reichstein Nielsen wrote:
> I store both loop information and labeled block information here, depending on
> what the block represents. Accessors below check the type before returning it.

I would prefer two fields. This way we could get rid of the marker-class too.

https://chromiumcodereview.appspot.com/9421035/diff/1/frog/leg/ssa/optimize.dart
File frog/leg/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/1/frog/leg/ssa/optimize.d...
frog/leg/ssa/optimize.dart:300: recursiveRemove(phi.usedBy[0]);
are you sure this can't yield an infinite loop?

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.d...
frog/leg/ssa/builder.dart:569: BreakHandlerImpl(this.breakTargets)
Instead of passing in breakTargets take the builder.
This looks too much like a non-shared variable.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.d...
frog/leg/ssa/builder.dart:942: BreakHandler beginLoopHeader(Node node) {
Instead of returning a BreakHandler make the BreakHandler a cons-structure
(stack) that returns the last head when closing.
Then add a field currentBreakHandler in the builder.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.d...
frog/leg/ssa/builder.dart:2117: currentBreakHandler = handler;
This looks like work that the resolver should have done.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.d...
frog/leg/ssa/builder.dart:2127: // no breaks anyway.
How can this be possible? I thought that the handler would be null then.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/codegen.d...
frog/leg/ssa/codegen.dart:384: void addLabel(SourceString label) {
to many addLabels and similar: sometimes with prefixed '$' sometimes with
label.toString, or with stringValue...
If they should be different make this clearer, if not share the code.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/codegen.d...
frog/leg/ssa/codegen.dart:398: // No block finishing with a 'throw' can have
more than
s/throw/break.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/codegen.d...
frog/leg/ssa/codegen.dart:488: visitBasicBlock(dominated[1]);
This doesn't look right. It seems that the if can dominate an arbitrary number
of blocks now (one for each break). And it's the if's job to visit them.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/tests/leg_only/sr...
File frog/tests/leg_only/src/LabelTest.dart (left):

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/tests/leg_only/sr...
frog/tests/leg_only/src/LabelTest.dart:54: foo: {          /// 10: compile-time
error
compile-time error = warning?

[ngeoffray, 2012-02-21 13:12:17]

DBC

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/optimize....
File frog/leg/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/optimize....
frog/leg/ssa/optimize.dart:311: if (!livePhis.contains(current)) {
Should we visit post dominator instead?

[Lasse Reichstein Nielsen, 2012-02-21 13:53:56]

https://chromiumcodereview.appspot.com/9421035/diff/1/frog/leg/ssa/nodes.dart
File frog/leg/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/1/frog/leg/ssa/nodes.dart...
frog/leg/ssa/nodes.dart:354: HBlockInformation blockInformation = null;
Done.

https://chromiumcodereview.appspot.com/9421035/diff/1/frog/leg/ssa/optimize.dart
File frog/leg/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/1/frog/leg/ssa/optimize.d...
frog/leg/ssa/optimize.dart:300: recursiveRemove(phi.usedBy[0]);
Not absolutely. I guess we could have recursive dependencies between phis in a
loop. I'll have to check.
Alternatively, I can just remove this phi as an input of the ones that uses it,
before removing itself (removePhi doesn't work if the phi has uses, and if we
can have cycles, we would need to break it somehow, so might as well do it
here).

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.d...
frog/leg/ssa/builder.dart:569: BreakHandlerImpl(this.breakTargets)
Will do.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.d...
frog/leg/ssa/builder.dart:942: BreakHandler beginLoopHeader(Node node) {
Done. Break handlers are now a list, and since they know the builder, they even
add and remove themselves from the list.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.d...
frog/leg/ssa/builder.dart:2117: currentBreakHandler = handler;
I agree, and agreed when doing the resolution, but someone wanted the label of a
labeled statement to resolve to that, not the the underlying statement.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.d...
frog/leg/ssa/builder.dart:2127: // no breaks anyway.
Some analysis might have turned a break into dead code, even though it was there
at resolution.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/codegen.dart
File frog/leg/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/codegen.d...
frog/leg/ssa/codegen.dart:384: void addLabel(SourceString label) {
There are two addLabels, one for optimized code (and being the default, it is in
SsaCodeGenerator), and one overriding it in SsaUnoptimizedCodeGenerator. I'll
add comments describing the difference.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/codegen.d...
frog/leg/ssa/codegen.dart:398: // No block finishing with a 'throw' can have
more than
On 2012/02/20 19:01:54, floitsch wrote:
> s/throw/break.

Done.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/codegen.d...
frog/leg/ssa/codegen.dart:488: visitBasicBlock(dominated[1]);
You are right. All this complication has been reduced to a much simpler, more
generic version (but with fewer asserts, since we don't really know much).

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/optimize....
File frog/leg/ssa/optimize.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/optimize....
frog/leg/ssa/optimize.dart:311: if (!livePhis.contains(current)) {
That might work if there is no cyclic dependency.
I'll do it this now, and at least not be doing any worse than before.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/tests/leg_only/sr...
File frog/tests/leg_only/src/LabelTest.dart (left):

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/tests/leg_only/sr...
frog/tests/leg_only/src/LabelTest.dart:54: foo: {          /// 10: compile-time
error
Not really, that's why it's gone.
Also because it's not specified what should happen.

[floitsch, 2012-02-22 13:17:20]

LGTM.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.d...
frog/leg/ssa/builder.dart:2117: currentBreakHandler = handler;
On 2012/02/21 13:53:56, Lasse Reichstein Nielsen wrote:
> I agree, and agreed when doing the resolution, but someone wanted the label of
a
> labeled statement to resolve to that, not the the underlying statement.

What I wanted:
a LabelElement that points to its AST node.
a mapping that gets the list of LabelElements for a block/loop.
But somebody didn't want to implement that.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.d...
frog/leg/ssa/builder.dart:2127: // no breaks anyway.
On 2012/02/21 13:53:56, Lasse Reichstein Nielsen wrote:
> Some analysis might have turned a break into dead code, even though it was
there
> at resolution.

As long as we don't have such an optimization, assert that it is not the case.
This comment is otherwise confusing.

https://chromiumcodereview.appspot.com/9421035/diff/7001/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/7001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:2141: HBasicBlock firstJoinBlock = null;
As discussed. let's get rid of firstJoinBlock and simply put the
labeledBlockInformation on the last merge-block.

[Lasse Reichstein Nielsen, 2012-02-22 14:12:24]

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.dart
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.d...
frog/leg/ssa/builder.dart:2117: currentBreakHandler = handler;
Touche. I'll consider redoing this in a different CL.

https://chromiumcodereview.appspot.com/9421035/diff/13/frog/leg/ssa/builder.d...
frog/leg/ssa/builder.dart:2127: // no breaks anyway.
As discussed offline, the break could be dead code from the beginning - it would
still be resolved by the resolver, but not be visited here in the builder.
If there really is no break for the target, we don't need the label, and without
the HLabeledBlockInformation, we don't need to create a join node at all. So,
much simplified.

https://chromiumcodereview.appspot.com/9421035/diff/7001/frog/leg/ssa/builder...
File frog/leg/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/9421035/diff/7001/frog/leg/ssa/builder...
frog/leg/ssa/builder.dart:2141: HBasicBlock firstJoinBlock = null;
Done.