Detect total switches to prevent type pollution by default cases.

Detect total switches to prevent type pollution by default cases.

BUG=
R=karlklose@google.com

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

[herhut, 2014-03-10 16:00:38]

I am looking particularly to feedback on how to pass the range information
around.

[karlklose, 2014-03-12 14:43:45]

LGTM.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:685:
generateStatements(info.statements.last);
Why did you change the order here?

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:686: if
(currentContainer.statements.isNotEmpty)
Please use curly braces.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:987: HInstruction
zap = zapInstruction;
Is this a performance optimization? I would prefer using zapInstruction
directly.

Could you create a helper and share it with the code in l. 1026-1028.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1043:
(this.compiler.backend as JavaScriptBackend).optimizer.ranges;
If you added a 'backend' field of type JavaScriptBackend instead of the
compiler, you wouldn't need the cast here (unless you want the explicit type
check in production mode ;-)

As a bonus, the expression fits in one line.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1080: if
(!node.isTotal && node.inputs[0].isInteger(compiler)) {
'node.inputs[0]' -> 'node.expression'.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1081: Range
switchRange = ranges[node.inputs[0]];
Ditto.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1092:
assert(input.isConstantInteger());
I think this assertion is unnecessary, the code below would already  throw in
checked mode if input is of a wrong type.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1096: (label <=
upper) &&
No need for parentheses.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1097: (label >=
lower)) {
Ditto.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/value_range_analyzer.dart
(right):

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/value_range_analyzer.dart:614:
(compiler.backend as JavaScriptBackend).optimizer.ranges = ranges;
Do you want the explicit type check here?

Consider using a temporary variable instead.

https://codereview.chromium.org/190763013/diff/1/tests/compiler/dart2js/switc...
File tests/compiler/dart2js/switch_empty_default_test.dart (right):

https://codereview.chromium.org/190763013/diff/1/tests/compiler/dart2js/switc...
tests/compiler/dart2js/switch_empty_default_test.dart:72: K:
I think we usually write the label as indented prefix of the case expression.

https://codereview.chromium.org/190763013/diff/1/tests/compiler/dart2js/switc...
tests/compiler/dart2js/switch_empty_default_test.dart:107: compileAndDoNotMatch(
SIMPLY_EMPTY, 'main', def);
Remove spaces before first arguments (here and below).

[floitsch, 2014-03-12 16:20:24]

DBC.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1093: IntConstant
constant = input.constant;
Pay attention that "-0.0" is a double-constant but counts as integer virtually
everywhere else.
I'm pretty sure that there are other cases where we get this wrong, though.

[herhut, 2014-03-13 13:27:45]

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:685:
generateStatements(info.statements.last);
On 2014/03/12 14:43:45, karlklose wrote:
> Why did you change the order here?

I need to generate the statements to see whether the case will be empty. I did
not want to first add the case to the list of cases and then remove it again, so
I delay adding it until after statement generation.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/codegen.dart:686: if
(currentContainer.statements.isNotEmpty)
On 2014/03/12 14:43:45, karlklose wrote:
> Please use curly braces.

Done.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/optimize.dart (right):

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:987: HInstruction
zap = zapInstruction;
On 2014/03/12 14:43:45, karlklose wrote:
> Is this a performance optimization? I would prefer using zapInstruction
> directly.
> 
> Could you create a helper and share it with the code in l. 1026-1028.

Done.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1043:
(this.compiler.backend as JavaScriptBackend).optimizer.ranges;
On 2014/03/12 14:43:45, karlklose wrote:
> If you added a 'backend' field of type JavaScriptBackend instead of the
> compiler, you wouldn't need the cast here (unless you want the explicit type
> check in production mode ;-)
> 
> As a bonus, the expression fits in one line.

I need the compiler, too, so I have decided to just add a getter for the backend
that has type JavaScriptBackend.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1080: if
(!node.isTotal && node.inputs[0].isInteger(compiler)) {
On 2014/03/12 14:43:45, karlklose wrote:
> 'node.inputs[0]' -> 'node.expression'.

Done.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1081: Range
switchRange = ranges[node.inputs[0]];
On 2014/03/12 14:43:45, karlklose wrote:
> Ditto.

Done.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1092:
assert(input.isConstantInteger());
On 2014/03/12 14:43:45, karlklose wrote:
> I think this assertion is unnecessary, the code below would already  throw in
> checked mode if input is of a wrong type.

You are probably right about the assert, it is a left over debugging vehicle.
The code was still wrong though, as it should not throw. Consider a switch over
integers that has some string cases, which is valid dart. I have modified the
code to not mark such cases as live.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1093: IntConstant
constant = input.constant;
On 2014/03/12 16:20:25, floitsch wrote:
> Pay attention that "-0.0" is a double-constant but counts as integer virtually
> everywhere else.
> I'm pretty sure that there are other cases where we get this wrong, though.

0.0 should be illegal in case statements (and hopefully soon will be, see
dartbug.com/17463)

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1096: (label <=
upper) &&
On 2014/03/12 14:43:45, karlklose wrote:
> No need for parentheses.

I just like the visual grouping they provide. To keep with the style, I've
removed them.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/optimize.dart:1097: (label >=
lower)) {
On 2014/03/12 14:43:45, karlklose wrote:
> Ditto.

Done.

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/ssa/value_range_analyzer.dart
(right):

https://codereview.chromium.org/190763013/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/ssa/value_range_analyzer.dart:614:
(compiler.backend as JavaScriptBackend).optimizer.ranges = ranges;
On 2014/03/12 14:43:45, karlklose wrote:
> Do you want the explicit type check here?
> 
> Consider using a temporary variable instead.

Done.

https://codereview.chromium.org/190763013/diff/1/tests/compiler/dart2js/switc...
File tests/compiler/dart2js/switch_empty_default_test.dart (right):

https://codereview.chromium.org/190763013/diff/1/tests/compiler/dart2js/switc...
tests/compiler/dart2js/switch_empty_default_test.dart:72: K:
On 2014/03/12 14:43:45, karlklose wrote:
> I think we usually write the label as indented prefix of the case expression.

Done.

https://codereview.chromium.org/190763013/diff/1/tests/compiler/dart2js/switc...
tests/compiler/dart2js/switch_empty_default_test.dart:107: compileAndDoNotMatch(
SIMPLY_EMPTY, 'main', def);
On 2014/03/12 14:43:45, karlklose wrote:
> Remove spaces before first arguments (here and below).

Done.

[herhut, 2014-03-20 10:31:21]

Committed patchset #2 manually as r34158 (presubmit successful).