From c604114dbbc89f680382469956996d960cca8126 Mon Sep 17 00:00:00 2001 From: Larry Ewing Date: Tue, 18 Aug 2026 20:57:46 -0500 Subject: [PATCH] [mono][interp] Fix mis-sorted SIMD intrinsic tables making entries unreachable lookup_intrins() searches the SIMD intrinsic tables with mono_binary_search and a strcmp comparator, which requires the tables be in strict ASCII order. Two tables had drifted out of order, silently making entries unreachable: the search returns -1, control falls through to "default: return FALSE", and the intrinsic is simply never emitted. There is no crash and no test failure, only a quiet fall back to the managed implementation - which is why this went unnoticed. Unreachable before this change: sri_vector128_methods: AsVector4 packedsimd_alias_methods: Store, StoreUnsafe, Subtract, SubtractSaturate Restoring the ordering makes Subtract, SubtractSaturate and AsVector4 emit their intrinsics. On browser-wasm under V8, SubtractSaturate had been running a fully managed scalar loop and now lowers to i8x16.sub_sat / i16x8.sub_sat: Vector128.SubtractSaturate 28.13 ns -> 0.84 ns (33.5x) Vector128.SubtractSaturate 30.73 ns -> 0.84 ns (36.6x) Store and StoreUnsafe are deliberately removed from the alias table rather than re-sorted, because sorting them back into place would have activated a latent memory-corruption bug that the mis-sort was masking. The alias path only renames cmethod_name, and emit_common_simd_epilogue assigns sregs in signature order with no reordering hook, but the operands are reversed: PackedSimd.Store(T* address, Vector128 source) Vector128.Store(this Vector128 source, T* destination) Vector128.StoreUnsafe(this Vector128 source, ref T destination) The existing param_count check does not catch this - lookup_packedsimd_intrinsic resolves Store as ANY, so it would have matched and passed the vector where the destination address is expected, dereferencing vector data as a pointer and writing 16 bytes through it. Removing the entries preserves exactly the current behavior. Making these genuinely intrinsifiable needs operand-swap support in the alias path and is left as a follow-up. Finally, add a check_intrins_sorted() assertion under ENABLE_CHECKED_BUILD so a future mis-sort fails loudly instead of quietly costing performance. 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: