Implement constant switch as JS switch.

Implement constant switch as JS switch.

This introduces a block-structure for constant switches.
Non-constant switches still use the old if-else-if implementation.

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

[Lasse Reichstein Nielsen, 2012-06-06 08:50:45]

https://chromiumcodereview.appspot.com/10544024/diff/1/tests/language/languag...
File tests/language/language_dart2js.status (left):

https://chromiumcodereview.appspot.com/10544024/diff/1/tests/language/languag...
tests/language/language_dart2js.status:66: label_test: Fail # switch cases are
not implemented
They still aren't, but the test doesn't test that.
I think it's just the outer label of a switch that used to be bad.

[Lasse Reichstein Nielsen, 2012-06-06 09:29:31]

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3013: // TODO(ngeoffray): Handle
switch-instruction in bailout code.
I hope it's simple if you know how :)

[floitsch, 2012-06-06 11:50:26]

LGTM.

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3032: for (Iterator<Node> iterator
= node.cases.iterator(); iterator.hasNext();) {
I would prefer a while loop.

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3037: if (labelOrCase is CaseMatch)
{
At the moment an assert would work too here, no?

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:652:
use(info.expression.conditionExpression, JSPrecedence.EXPRESSION_PRECEDENCE);
80chars.

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/nodes.dart:1485: * Its block has one succeessor
per constant, and one for the default.
successor.

[ngeoffray, 2012-06-06 12:03:46]

LGTM

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3058: compiler.reportWarning(node,
'Missing break at end of switch case');
I don't think the codegen should emit a warning.

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3060: compiler.findHelper(const
SourceString("getFallThroughError"));
Maybe move that call out of the loop?

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3078: if (!isAborted()) {
Should this check be in the one line 3090? You're adding joinBlock as a
successor, but if we fail check 3090 the joinBlock will not be inserted in the
graph.

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3083: if (!hasDefault) {
ditto

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/nodes.dart:1493: HBasicBlock get defaultTarget()
=> block.successors.last();
What if the switch does not have a default? Is that the join block then? Please
add a comment.

[Lasse Reichstein Nielsen, 2012-06-06 13:01:21]

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3032: for (Iterator<Node> iterator
= node.cases.iterator(); iterator.hasNext();) {
On 2012/06/06 11:50:26, floitsch wrote:
> I would prefer a while loop.

Done.

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3037: if (labelOrCase is CaseMatch)
{
No, we still parse the labels, we just don't do anything with them.

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3058: compiler.reportWarning(node,
'Missing break at end of switch case');
True. Something earlier should do that. 
I'll remove it here, and leave it to someone else to add it where it belongs :)

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3060: compiler.findHelper(const
SourceString("getFallThroughError"));
Reasonable. Done.

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3078: if (!isAborted()) {
If this test succeedes, it adds an element to caseLocals, so the test in 3090
will succeede. I'll add a comment and assert that  caseLocals.length ==
joinBlock.predecessors.length .

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3083: if (!hasDefault) {
And ditto.

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/codegen.dart:652:
use(info.expression.conditionExpression, JSPrecedence.EXPRESSION_PRECEDENCE);
On 2012/06/06 11:50:26, floitsch wrote:
> 80chars.

Done.

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/nodes.dart (right):

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/nodes.dart:1485: * Its block has one succeessor
per constant, and one for the default.
On 2012/06/06 11:50:26, floitsch wrote:
> successor.

Done.

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/nodes.dart:1493: HBasicBlock get defaultTarget()
=> block.successors.last();
It is. Comment added.

[floitsch, 2012-06-06 14:32:00]

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10544024/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3037: if (labelOrCase is CaseMatch)
{
On 2012/06/06 13:01:21, Lasse Reichstein Nielsen wrote:
> No, we still parse the labels, we just don't do anything with them.

But isn't there a "return false" in line 3009 for this?

[Lasse Reichstein Nielsen, 2012-06-07 12:03:05]

http://codereview.chromium.org/10544024/diff/1/lib/compiler/implementation/ss...
File lib/compiler/implementation/ssa/builder.dart (right):

http://codereview.chromium.org/10544024/diff/1/lib/compiler/implementation/ss...
lib/compiler/implementation/ssa/builder.dart:3037: if (labelOrCase is CaseMatch)
{
True, there is. Let's not make the assumption in more than one place.