From c2773aab9800fa1667e82faad4fe79f22ae3c5ab Mon Sep 17 00:00:00 2001 From: Larry Ewing Date: Wed, 19 Aug 2026 07:26:43 -0500 Subject: [PATCH] [mono][interp] Fix mis-sorted SIMD intrinsic tables making entries unreachable (#132500) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem `lookup_intrins()` in `transform-simd.c` resolves method names with `mono_binary_search` + `strcmp`, which requires the tables to be in strict ASCII order. Two tables had drifted out of order, so binary search never reached certain entries: lookup returns `-1`, the `switch` falls to `default: return FALSE`, and the intrinsic is silently never emitted. The failure is invisible — no crash, no assert, no test failure. The call just falls back to the managed implementation and runs orders of magnitude slower. That is why this went unnoticed. Entries unreachable on `main`: | Table | Unreachable entries | |---|---| | `sri_vector128_methods` | `AsVector4` | | `packedsimd_alias_methods` | `Store`, `StoreUnsafe`, `Subtract`, `SubtractSaturate` | ## Changes 1. **`sri_vector128_methods`** — reorder `SN_AsVector, SN_AsVector4, SN_AsVector128` → `SN_AsVector, SN_AsVector128, SN_AsVector4`. 2. **`packedsimd_alias_methods`** — move `SN_Sqrt, SN_SquareRoot` from after `SN_SubtractSaturate` to before `SN_Subtract`, restoring the sort invariant and making `Subtract`/`SubtractSaturate` reachable. 3. **`packedsimd_alias_methods`** — *remove* `SN_Store` / `SN_StoreUnsafe` (and the now-orphaned `case` block), replaced with a comment explaining why. See below. 4. **New `check_intrins_sorted()`** under `ENABLE_CHECKED_BUILD`, called from `lookup_intrins()`, so any future mis-sort fails loudly instead of silently degrading. `ENABLE_CHECKED_BUILD` is on for any Debug mono build, so this gets broad coverage. Cost is at most ~46 short `strcmp`s per SIMD resolution at transform time. ## Why `Store`/`StoreUnsafe` are removed rather than sorted into place This is the part worth reviewing carefully. **Naively re-sorting the table would have activated a latent memory-corruption bug**, which is presumably why nobody noticed these were dead. The operands are reversed between the two APIs: ```c // interp-simd.c:799 — expects (address, vector) interp_packedsimd_store (res, addr_of_addr, vec) → **(v128_t **)addr_of_addr = *(v128_t *)vec; ``` | API | Signature | Matches helper? | |---|---|---| | `PackedSimd.Store(T* address, Vector128 source)` | (address, vector) | ✅ | | `Vector128.Store(this Vector128 source, T* destination)` | (vector, address) | ❌ reversed | | `Vector128.StoreUnsafe(this Vector128 source, ref T destination)` | (vector, address) | ❌ reversed | The alias path only *renames* `cmethod_name`; `emit_common_simd_epilogue` then assigns `sregs[i] = sp[i].var` in signature order with no reordering hook. The existing `param_count != 2` guard is necessary but not sufficient — `lookup_packedsimd_intrinsic("Store", ...)` matches the `Store, ANY` entry regardless. The result would be vector data dereferenced as a pointer and 16 bytes written through it. Removing the entries preserves `main`'s behavior exactly (they are unreachable today) while making that explicit and keeping the table sorted. Making these genuinely intrinsifiable is worth doing — measured ~6.4x on the interpreter — but it needs an operand-swap mechanism in the emit path, which is a feature rather than a bug fix and belongs in its own change. ## Why the newly-reachable entries are safe - **`Subtract`** — identical `(Vector128 left, Vector128 right)` shape in both APIs. Covers D1/D2/D4/D8/R4/R8 (including `nint`/`nuint` → D4 on wasm32 via `resolve_native_size`). - **`SubtractSaturate`** — exists only for I1/U1/I2/U2. For other types `packedsimd_type_matches` hits `default: return FALSE` and falls back to managed. Structurally identical to the already-reachable `AddSaturate`. - **`AsVector4`** — only `Vector128.AsVector4(this Vector128)` reaches the switch; return and argument are both 16 bytes, so it emits `MINT_MOV_VT` 16, exactly the `Unsafe.BitCast` it replaces. The `Plane`/`Quaternion`/`Vector2`/`Vector3` overloads return `FALSE` earlier (non-`GENERICINST`). - Neither `Add` nor `Subtract` nor `SubtractSaturate` has scalar overloads (only `Multiply` does, and `SN_Multiply` already guards with `scalar_arg != -1`), so the absent `scalar_arg` check is not exploitable here. `Sqrt`/`SquareRoot` happened to be reachable despite their misplacement — the search path landed on them. Moving them is invariant hygiene, not a behavior change. ## Measurements browser-wasm / V8, interpreter, dependency-chained loops, best of 5 after 2 warmups: | Operation | Before | After | Speedup | |---|---|---|---| | `Vector128.SubtractSaturate` | 28.13 ns | 0.84 ns | **33.5x** | | `Vector128.SubtractSaturate` | 30.73 ns | 0.84 ns | **36.6x** | | `Vector128.Subtract` (static) | 0.69 ns | 0.68 ns | unchanged | `SubtractSaturate` was running a fully-managed scalar loop and now lowers to `i8x16.sub_sat` / `i16x8.sub_sat`. Static `Subtract` is unchanged because its body is `a - b`, already intrinsified through `op_Subtraction` and inlined — the table entry was redundant for it. ## Validation - Audit script replaying `mono_binary_search` over all 5 tables: all sorted, all entries reachable. - **Checked build** with the new assert active: builds clean, **13021/13021** `System.Runtime.Intrinsics` tests pass on browser-wasm, assert never fires. - **Shipping config**: 0 warnings / 0 errors, **13021/13021**. - **Native osx-arm64 mono**: 0 warnings / 0 errors. - IR dump confirms emission: `simd_intrins_p_pp [80 <- 80 96], 166` → `SubtractSaturateI2`. ## Note on merge order This also touches `sri_vector128_methods`, which overlaps an incidental reorder in #132499. Trivial conflict; whichever merges second needs a rebase. It could not be scoped out of this change — the new sortedness assert fires on the pre-existing `AsVector4` misplacement, so the fix has to land together with the assert. > [!NOTE] > This pull request description was generated with the assistance of GitHub Copilot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3ebb2f49-c9fc-4c28-9755-c51b9deac735 --- src/mono/mono/mini/interp/transform-simd.c | 39 ++++++++++++++++------ 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/src/mono/mono/mini/interp/transform-simd.c b/src/mono/mono/mini/interp/transform-simd.c index b41bfc796d20fd..39f08cccff6564 100644 --- a/src/mono/mono/mini/interp/transform-simd.c +++ b/src/mono/mono/mini/interp/transform-simd.c @@ -43,9 +43,29 @@ simd_intrinsic_compare_by_name (const void *key, const void *value) return strcmp ((const char*)key, method_name (*(guint16*)value)); } +#ifdef ENABLE_CHECKED_BUILD +// The tables below are searched with mono_binary_search, so an out-of-order entry silently makes +// itself and potentially its neighbors unreachable - the intrinsic is never emitted and we fall +// back to the managed implementation with no other visible symptom. Validate the invariant here +// so that a mis-sorted table fails loudly in checked builds instead of quietly losing performance. +static void +check_intrins_sorted (guint16 *intrinsics, int size) +{ + int count = size / sizeof (guint16); + for (int i = 1; i < count; i++) { + const char *prev = method_name (intrinsics [i - 1]), *cur = method_name (intrinsics [i]); + g_assertf (strcmp (prev, cur) < 0, + "interp SIMD intrinsic table is not in ASCII order: '%s' must not precede '%s'", prev, cur); + } +} +#endif + static int lookup_intrins (guint16 *intrinsics, int size, const char *cmethod_name) { +#ifdef ENABLE_CHECKED_BUILD + check_intrins_sorted (intrinsics, size); +#endif guint16 *result = mono_binary_search (cmethod_name, intrinsics, size / sizeof (guint16), sizeof (guint16), &simd_intrinsic_compare_by_name); if (result == NULL) @@ -74,8 +94,8 @@ static guint16 sri_vector128_methods [] = { SN_AsUInt32, SN_AsUInt64, SN_AsVector, - SN_AsVector4, SN_AsVector128, + SN_AsVector4, SN_ConditionalSelect, SN_Create, SN_CreateScalar, @@ -173,12 +193,15 @@ static guint16 packedsimd_alias_methods [] = { SN_ShiftLeft, SN_ShiftRightArithmetic, SN_ShiftRightLogical, - SN_Store, - SN_StoreUnsafe, - SN_Subtract, - SN_SubtractSaturate, SN_Sqrt, SN_SquareRoot, + // NOTE: Store/StoreUnsafe are deliberately absent. PackedSimd.Store's operands are reversed + // relative to Vector128's - PackedSimd.Store(T* address, Vector128 source) versus + // Vector128.Store(this Vector128 source, T* destination) - and this path only renames + // the method, with emit_common_simd_epilogue assigning sregs in signature order. Aliasing + // them would pass the vector where the destination address is expected and corrupt memory. + SN_Subtract, + SN_SubtractSaturate, SN_Truncate, SN_WidenLower, SN_WidenUpper, @@ -1251,12 +1274,6 @@ emit_sri_packedsimd (TransformData *td, MonoMethod *cmethod, MonoMethodSignature case SN_SquareRoot: cmethod_name = "Sqrt"; break; - case SN_Store: - case SN_StoreUnsafe: - if (csignature->param_count != 2) - return FALSE; - cmethod_name = "Store"; - break; case SN_Add: case SN_AddSaturate: case SN_AndNot: