dart-lang/sdk
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

[dart2wasm] Revert switch-case optimizations that introduced a bug.

Browse Source 
This reverts commit b65435017d
  This reverts commit 2254755d65

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>
This commit is contained in:
Martin Kustermann
2025-07-31 06:13:38 -07:00
committed by Commit Queue
parent bd0fa88e0b
commit 21f7ffc699
2 changed files with 20 additions and 178 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/dart2wasm/lib/code_generator.dart
+20 -176
View File
@@ -1424,44 +1424,29 @@ abstract class AstCodeGenerator
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 @@ class SwitchBackwardJumpInfo {
: 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 @@ class SwitchInfo {
/// 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 @@ class SwitchInfo {
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 @@ class SwitchInfo {
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;
pkg/dart2wasm/lib/kernel_nodes.dart
-2
View File
@@ -57,8 +57,6 @@ mixin KernelNodes {
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");