Uh oh!
There was an error while loading. Please reload this page.
[release/11.0-rc1] [mono][interp] Fix mis-sorted SIMD intrinsic tables making entries unreachable - #132525
Merged
Conversation
…reachable (#132500) ## 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<T> source)` | (address, vector) | ✅ | | `Vector128.Store<T>(this Vector128<T> source, T* destination)` | (vector, address) | ❌ reversed | | `Vector128.StoreUnsafe<T>(this Vector128<T> 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<T> left, Vector128<T> 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<float>)` 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<short>` | 28.13 ns | 0.84 ns | **33.5x** | | `Vector128.SubtractSaturate<byte>` | 30.73 ns | 0.84 ns | **36.6x** | | `Vector128.Subtract<int>` (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
github-actionsBot
requested review from
BrzVlad, kotlarmilos, steveisok and vitek-karas
as code ownersAugust 19, 2026 15:33
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
lewing
approved these changes
Aug 19, 2026
tannergooding
approved these changes
Aug 19, 2026
Uh oh!
There was an error while loading. Please reload this page.
lewing
enabled auto-merge (squash)
August 20, 2026 14:27
Uh oh!
There was an error while loading. Please reload this page.
lewing added a commit
that referenced
this pull request
Aug 20, 2026
…afe on wasm (#132575) Manual backport of #132502 to `release/11.0-rc1`. ## Why this needed a manual backport The automatic cherry-pick conflicted in two files: - **`transform-simd.c`** — resolved by first landing the prerequisite, #132525 (backport of #132500). Once that merged, this file auto-merged with no hand edits. - **`Vector128Tests.cs`** — a pure placement conflict. #132502's tests were appended after `Vector128GetElementVariableOutOfRangeTest`, which comes from #132499 and is not on this branch. The three Store tests are self-contained, so they were appended at the end of the class instead. No #132499 tests were pulled in. Every added and removed line in this commit is byte-identical to upstream `be1c680` — only hunk headers and line numbers differ. ## Depends on #132525 This backport is only effective because #132525 already merged. On `release/11.0-rc1` before that fix, `packedsimd_alias_methods` was mis-sorted, and since `lookup_intrins()` binary-searches the table, `Store`, `StoreUnsafe`, `Subtract` and `SubtractSaturate` were all unreachable. Simulating `mono_binary_search` against the pre-#132525 table confirms this — so #132502 on its own would have been a silent no-op. With #132525 merged, the resulting table is strictly sorted and all 49 entries resolve; `Store` and `StoreUnsafe` land at indices 29 and 30. ## Original change `PackedSimd.Store(T* address, Vector128<T> source)` takes its operands in the opposite order from `Vector128.Store(this Vector128<T> source, T* destination)`, and the alias path only renames the method — `emit_common_simd_epilogue` assigns sregs in signature order. This adds them to the alias table and swaps the two sregs after the epilogue, guarded on the shape actually being a store (two parameters, void return, raw address as the second parameter). The three-argument `StoreUnsafe(source, destination, elementOffset)` has no PackedSimd counterpart and still falls back to managed code. `interp_packedsimd_store` also switches from assigning through a `v128_t*` to `wasm_v128_store`, which does not claim 16-byte alignment that neither API guarantees. Upstream measured ~6.2x on `Vector128.Store<int>` and ~6.3x on `Vector128.StoreUnsafe<int>` under the interpreter. ## Local validation - `./build.sh clr+libs -rc release` — 0 errors, 0 warnings - `System.Runtime.Intrinsics.Tests` — **13011/13011 passing, 0 failed**, including the three new tests: `Vector128StoreUnalignedTest`, `Vector128StoreUnsafeUnalignedTest`, `Vector128StoreUnsafeElementOffsetUnalignedTest` The count is 13011 rather than upstream's 13024 because the #132499 and #132496 tests are not on this branch. Not validated locally: the `interp-simd.c` change is wasm-only, so a native osx-arm64 build does not exercise it. It is byte-identical to the upstream commit, which passed full CI. > [!NOTE] > This pull request was authored with the assistance of GitHub Copilot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3ebb2f49-c9fc-4c28-9755-c51b9deac735
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #132500 to release/11.0-rc1
/cc @lewing
Customer Impact
[Select one or both of the boxes. Describe how this issue impacts customers, citing the expected and actual behaviors and scope of the issue. If customer-reported, provide the issue number.]
Regression
[If yes, specify when the regression was introduced. Provide the PR or commit if known.]
Testing
[How was the fix verified? How was the issue missed previously? What tests were added?]
Risk
[High/Medium/Low. Justify the indication by mentioning how risks were measured and addressed.]
IMPORTANT: If this backport is for a servicing release, please verify that:
release/X.0-staging, notrelease/X.0.release/X.0(no-stagingsuffix).Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.