[dart2wasm] Revert switch-case optimizations that introduced a bug.
This reverts commit b65435017da9dd4a937f6081c5bedee5ca926708
This reverts commit 2254755d65a4b8c9bcce6489de9dee5233ac3077
The switch-case optimizations introduced a bug that is surfaced when
running the dart2wasm self-compilation test in [0]. With that test
running
```
% python3 tools/test.py -n unittest-mac pkg/dart2wasm/test/self_compile_test
```
will fail with
```
...
Exception in StaticInvocation at file:///FakeSdkRoot/sdk/lib/_internal/wasm/lib/boxed_double.dart:346:29
Bad state: Unhandled WasmArray intrinsic: StaticIntrinsic.wasmArrayIndex
at module0.Error._throwWithCurrentStackTrace (wasm://wasm/module0-0121906a:wasm-function[160]:0xd0d76)
at module0.AstCodeGenerator.visitStaticInvocation (checked entry) (wasm://wasm/module0-0121906a:wasm-function[6304]:0x1668e5)
at module0._TreeVisitor1Default&Object&TreeVisitor1DefaultMixin&ExpressionVisitor1DefaultMixin.visitStaticInvocation (checked entry) (wasm://wasm/module0-0121906a:wasm-function[6306]:0x167b61)
at module0.StaticInvocation.accept1 (wasm://wasm/module0-0121906a:wasm-function[6297]:0x165f5f)
at module0.AstCodeGenerator.translateExpression (wasm://wasm/module0-0121906a:wasm-function[1742]:0xfcd1b)
at module0.AstCodeGenerator.visitEqualsCall (checked entry) (wasm://wasm/module0-0121906a:wasm-function[6960]:0x1777e4)
at module0.EqualsCall.accept1 (wasm://wasm/module0-0121906a:wasm-function[6944]:0x1773b4)
at module0.AstCodeGenerator.translateExpression (wasm://wasm/module0-0121906a:wasm-function[1742]:0xfcd1b)
/Users/kustermann/src/dart-sdk/sdk/pkg/dart2wasm/bin/run_wasm.js:346: [object WebAssembly.Exception]
```
The switch-case in the intrinsifier seems to be miscompiled.
Change-Id: I3bfe8887fa133573379c32d52e15769a4e6db43e
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/443082
Commit-Queue: Martin Kustermann <kustermann@google.com>
Reviewed-by: Ömer Ağacan <omersa@google.com>
diff --git a/pkg/dart2wasm/lib/code_generator.dart b/pkg/dart2wasm/lib/code_generator.dart
index e35e8ab..455113a 100644
--- a/pkg/dart2wasm/lib/code_generator.dart
+++ b/pkg/dart2wasm/lib/code_generator.dart
@@ -1424,44 +1424,29 @@
b.end();
}
- final brTable = switchInfo.brTable;
- if (brTable != null) {
- // Map each entry in the range to the appropriate jump table entry.
- final indexBlocks = <w.Label>[];
- final caseMap = brTable.caseMap;
- final defaultLabel =
- defaultCase != null ? switchLabels[defaultCase]! : doneLabel;
- for (int i = 0; i < brTable.rangeSize; i++) {
- final c = caseMap[i];
- indexBlocks.add(c == null ? defaultLabel : switchLabels[c]!);
- }
- brTable.brTableExpr(switchValueNonNullableLocal);
- b.br_table(indexBlocks, defaultLabel);
- } else {
- // Compare against all case values
- for (SwitchCase c in node.cases) {
- for (Expression exp in c.expressions) {
- if (exp is NullLiteral ||
- exp is ConstantExpression && exp.constant is NullConstant) {
- // Null already checked, skip
- } else {
- switchInfo.compare(
- switchValueNonNullableLocal,
- () => translateExpression(exp, switchInfo.nonNullableType),
- );
- b.br_if(switchLabels[c]!);
- }
+ // Compare against all case values
+ for (SwitchCase c in node.cases) {
+ for (Expression exp in c.expressions) {
+ if (exp is NullLiteral ||
+ exp is ConstantExpression && exp.constant is NullConstant) {
+ // Null already checked, skip
+ } else {
+ switchInfo.compare(
+ switchValueNonNullableLocal,
+ () => translateExpression(exp, switchInfo.nonNullableType),
+ );
+ b.br_if(switchLabels[c]!);
}
}
+ }
- // No explicit cases matched
- if (node.isExplicitlyExhaustive) {
- b.unreachable();
- } else {
- w.Label defaultLabel =
- defaultCase != null ? switchLabels[defaultCase]! : doneLabel;
- b.br(defaultLabel);
- }
+ // No explicit cases matched
+ if (node.isExplicitlyExhaustive) {
+ b.unreachable();
+ } else {
+ w.Label defaultLabel =
+ defaultCase != null ? switchLabels[defaultCase]! : doneLabel;
+ b.br(defaultLabel);
}
// Emit case bodies
@@ -4312,47 +4297,6 @@
: defaultLoopLabel = null;
}
-/// Info needed to represent a switch statement using a br_table instruction.
-///
-/// This is used for switches on integers and enums (using their indicies). We
-/// map each option to an index in the jump table and then jump directly to the
-/// target case. This is much faster than iteratively comparing each cases's
-/// expression to the switch expression.
-///
-/// Sometimes switches over ranges of ints are sparse enough that a table would
-/// bloat the code compared to the iterative comparison.
-class BrTableInfo {
- // This is the minimum ratio of br_table indicies to actual mapped cases. If
- // this gets too large then most of the br_table indicies are unmapped and the
- // extra code size is not worth using a br_table.
- static const double _minTableSparseness = 2;
- // This is the maximum size where it's always worth it to use a br_table.
- // If the br_table is bigger than this then we start to check sparseness.
- // Below this point we always accept the potential code size hit.
- static const int _maxSparseSize = 50;
-
- final int rangeSize;
- final void Function(w.Local switchExprLocal) brTableExpr;
- final Map<int, SwitchCase> caseMap;
-
- BrTableInfo._(this.rangeSize, this.caseMap, this.brTableExpr)
- : assert(!isTooSparse(rangeSize, caseMap));
-
- /// Heuristically validate whether the provided table would be too sparse and
- /// if so return null. Otherwise return the expected table.
- static BrTableInfo? build(int rangeSize, Map<int, SwitchCase> caseMap,
- void Function(w.Local switchExprLocal) brTableExpr) {
- // Validate the table density and size is worth putting into a br_table.
- if (isTooSparse(rangeSize, caseMap)) return null;
-
- return BrTableInfo._(rangeSize, caseMap, brTableExpr);
- }
-
- static bool isTooSparse(int rangeSize, Map<int, SwitchCase> caseMap) =>
- (rangeSize / caseMap.length) > _minTableSparseness &&
- rangeSize > _maxSparseSize;
-}
-
class SwitchInfo {
/// Non-nullable Wasm type of the `switch` expression. Used when the
/// expression is not nullable, and after the null check.
@@ -4379,11 +4323,6 @@
/// The `null: ...` case, if exists.
late final SwitchCase? nullCase;
- /// Info needed to compile this switch statement into a wasm br_table. If null
- /// this switch statement should not use a br_table and should use comparison
- /// based case matching instead.
- BrTableInfo? brTable;
-
SwitchInfo(AstCodeGenerator codeGen, SwitchStatement node) {
final translator = codeGen.translator;
@@ -4514,39 +4453,6 @@
nonNullableType = w.NumType.i64;
nullableType =
translator.classInfo[translator.boxedIntClass]!.nullableType;
-
- // Calculate the range covered by the cases and create the jump table.
- int? minValue;
- int? maxValue;
- Map<int, SwitchCase> caseMap = {};
- for (final c in node.cases) {
- for (final e in c.expressions) {
- final value = e is IntLiteral
- ? e.value
- : ((e as ConstantExpression).constant as IntConstant).value;
- caseMap[value] = c;
- if (minValue == null || value < minValue) minValue = value;
- if (maxValue == null || value > maxValue) maxValue = value;
- }
- }
- if (maxValue != null) {
- final range = maxValue - minValue! + 1;
- if (minValue != 0) {
- caseMap = caseMap.map((i, c) => MapEntry(i - minValue!, c));
- }
- brTable = BrTableInfo.build(range, caseMap, (switchExprLocal) {
- codeGen.b.local_get(switchExprLocal);
- // Normalize the value on 0 if necessary.
- if (minValue != 0) {
- codeGen.b.i64_const(minValue!);
- codeGen.b.i64_sub();
- }
- // Now that we've normalized to 0 it should be safe to switch to i32.
- codeGen.b.i32_wrap_i64();
- });
- }
-
- // Provide a compare as a fallback in case the range is too sparse.
compare = (switchExprLocal, pushCaseExpr) {
codeGen.b.local_get(switchExprLocal);
pushCaseExpr();
@@ -4561,68 +4467,6 @@
pushCaseExpr();
codeGen.call(translator.jsStringEquals.reference);
};
- } else if (switchExprClass.isEnum) {
- // If this is an applicable switch over enums, create a jump table.
- bool isValid = true;
- final caseMap = <int, SwitchCase>{};
- int? minIndex;
- int maxIndex = 0;
- outer:
- for (final c in node.cases) {
- for (final e in c.expressions) {
- if (e is! ConstantExpression) {
- isValid = false;
- break outer;
- }
- final constant = e.constant;
- if (constant is! InstanceConstant) {
- isValid = false;
- break outer;
- }
- if (constant.classNode != switchExprClass) {
- isValid = false;
- break outer;
- }
- final enumIndex =
- (constant.fieldValues[translator.enumIndexField.fieldReference]
- as IntConstant)
- .value;
- caseMap[enumIndex] = c;
- if (enumIndex > maxIndex) maxIndex = enumIndex;
- if (minIndex == null || enumIndex < minIndex) minIndex = enumIndex;
- }
- }
-
- if (isValid && minIndex != null) {
- final range = maxIndex - minIndex + 1;
- brTable = BrTableInfo.build(range, caseMap, (switchExprLocal) {
- codeGen.b.local_get(switchExprLocal);
- codeGen.call(translator.enumIndexField.getterReference);
- if (minIndex != 0) {
- codeGen.b.i64_const(minIndex!);
- codeGen.b.i64_sub();
- }
- // Now that we've normalized to 0 it should be safe to switch to i32.
- codeGen.b.i32_wrap_i64();
- });
- }
-
- if (brTable == null) {
- // Object identity switch
- nonNullableType = translator.topTypeNonNullable;
- nullableType = translator.topType;
- } else {
- nonNullableType =
- translator.classInfo[switchExprClass]!.nonNullableType;
- nullableType = translator.classInfo[switchExprClass]!.nullableType;
- }
-
- // Set compare anyway for state machine handling
- compare = (switchExprLocal, pushCaseExpr) {
- codeGen.b.local_get(switchExprLocal);
- pushCaseExpr();
- codeGen.call(translator.coreTypes.identicalProcedure.reference);
- };
} else {
// Object identity switch
nonNullableType = translator.topTypeNonNullable;
diff --git a/pkg/dart2wasm/lib/kernel_nodes.dart b/pkg/dart2wasm/lib/kernel_nodes.dart
index 6a35225..c4d8f60 100644
--- a/pkg/dart2wasm/lib/kernel_nodes.dart
+++ b/pkg/dart2wasm/lib/kernel_nodes.dart
@@ -57,8 +57,6 @@
late final Class typeErrorClass = index.getClass("dart:core", "_TypeError");
late final Class javaScriptErrorClass =
index.getClass("dart:core", "_JavaScriptError");
- late final Field enumIndexField =
- index.getField('dart:core', '_Enum', 'index');
// dart:core runtime type classes
late final Class typeClass = index.getClass("dart:core", "_Type");
