Uh oh!
There was an error while loading. Please reload this page.
[mono][interp] Intrinsify Vector128 GetElement/WithElement on wasm - #132499
Conversation
The interpreter did not recognize Vector128<T>.GetElement or Vector128<T>.WithElement, so every lane access fell back to the managed implementation. On browser-wasm that costs a 16 byte vector copy to the stack plus address arithmetic and an indirect load for a single lane read. System.Numerics uses these APIs heavily (roughly 120 GetElement and 66 WithElement call sites), so the overhead shows up in Matrix4x4 and Quaternion code. The PackedSimd ExtractScalar/ReplaceScalar interpreter intrinsics already implement exactly this operation, so map the two methods onto them rather than adding new opcodes. No new MINT opcodes, interp-simd helpers, or jiterpreter changes are needed; the existing simdExtractTable and simdReplaceTable already lower these intrinsics to wasm extract_lane and replace_lane. The public APIs throw ArgumentOutOfRangeException for an out of range lane, but interpreter SIMD intrinsics are plain C helpers that cannot raise a managed exception, and the underlying helpers mask the lane index rather than checking it. Intrinsify only when the index is a compile time constant that is provably in [0, laneCount), which makes the mask a no-op. Every other case, a variable index or an out of range constant, returns FALSE so the managed implementation runs and throws. That restriction is also what the jiterpreter requires, since the wasm extract_lane and replace_lane opcodes take the lane as an immediate. MONO_TYPE_CHAR and any non primitive fall through to the default and are not intrinsified. ExtractScalar I1/U1/I2/U2 sign or zero extend to match the interpreter's I4 stack slot; WithElement uses the signedness agnostic ReplaceScalarD* helpers. Also reorder SN_AsVector128 before SN_AsVector4 in sri_vector128_methods. That table is searched with mono_binary_search, so it must be strictly sorted. The two entries were already inverted, and because probe positions depend on the array length, adding entries would have made AsVector128 unreachable. Measured on browser-wasm Release with V8: Vector128<float>.WithElement(2) 2.875 -> 1.775 ns -38.3% Vector128<byte>.GetElement(11) 1.267 -> 1.047 ns -17.4% Vector128<int>.GetElement(3) 1.288 -> 1.075 ns -16.5% Matrix4x4.CreateFromQuaternion 39.422 -> 32.350 ns -17.9% Quaternion.CreateFromRotationMatrix 42.344 -> 38.078 ns -10.1% Matrix4x4.Decompose 253.540 -> 242.150 ns -4.5% Vector3.Transform(matrix) 69.200 -> 67.268 ns -2.8% dotnet.native.wasm grows by 329 bytes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3ebb2f49-c9fc-4c28-9755-c51b9deac735
|
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. |
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
There was a problem hiding this comment.
Pull request overview
This PR updates the Mono interpreter’s SIMD intrinsic expansion to recognize Vector128<T>.GetElement and Vector128<T>.WithElement and lower them (on HOST_BROWSER/HOST_WASI) to existing PackedSimd extract/replace scalar helpers when the lane index is a provable in-range constant, while preserving ArgumentOutOfRangeException behavior by falling back to the managed implementation otherwise.
Changes:
- Fixes
sri_vector128_methods[]ordering to keepmono_binary_searchcorrect and addsGetElement/WithElementto the searchable method set. - Adds wasm-only intrinsic expansion for
SN_GetElement/SN_WithElement, gated on a basic-block-local constant-index check. - Adds new unit tests that explicitly exercise constant-index behavior and out-of-range/variable-index exception behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/mono/mono/mini/interp/transform-simd.c | Fixes intrinsic lookup table ordering and adds wasm/WASI lowering for GetElement/WithElement with a constant in-range lane guard. |
| src/mono/mono/mini/interp/simd-methods.def | Adds GetElement and WithElement to the SIMD method name table used for intrinsic lookup. |
| src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector128Tests.cs | Adds tests covering constant-index extraction/replacement and ensuring out-of-range/variable indices still throw. |
…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
Uh oh!
There was an error while loading. Please reload this page.
…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
Summary
The Mono interpreter did not recognize
Vector128<T>.GetElementorVector128<T>.WithElement, so every lane access fell back to the managed implementation. On browser-wasm that costs a 16-byte vector copy to the stack, address arithmetic, and an indirect load — for a single lane read.System.Numericsleans on these APIs heavily (~120GetElementand ~66WithElementcall sites), so the overhead is visible inMatrix4x4andQuaternioncode.The PackedSimd
ExtractScalar/ReplaceScalarinterpreter intrinsics already implement exactly this operation, so this maps the two methods onto them. No new MINT opcodes, no new interp-simd helpers, and no jiterpreter changes — the existingsimdExtractTable/simdReplaceTablealready lower these to wasmextract_lane/replace_lane.Preserving
ArgumentOutOfRangeExceptionThe public APIs throw
ArgumentOutOfRangeExceptionfor an out-of-range lane, but interpreter SIMD intrinsics are plain C helpers that cannot raise a managed exception. Worse, the underlying helper masks the lane index rather than checking it:That reads only the low byte and masks, so a naive lowering would make
GetElement(-1)silently return lane 15 instead of throwing.So this intrinsifies only when the index is a compile-time constant provably in
[0, laneCount), which makes the mask a provable no-op. Every other case — a variable index, or an out-of-range constant — returnsFALSEso the managed implementation runs and throws.That restriction is also what the jiterpreter needs independently, since the wasm
extract_lane/replace_laneopcodes take the lane as an immediate. A non-constant lane would truncate the trace rather than produce vectorized code. (Trace truncation is a perf loss only, never a behavior change.)MONO_TYPE_CHARand any non-primitive fall through todefault:and are not intrinsified.ExtractScalarI1/U1/I2/U2 sign/zero-extend to match the interpreter's I4 stack slot;WithElementuses the signedness-agnosticReplaceScalarD*helpers.Table ordering fix (required, not cosmetic)
sri_vector128_methods[]is searched withmono_binary_search, so it must be strictly sorted.SN_AsVector4andSN_AsVector128were already inverted onmain(leavingAsVector4unreachable). Because probe positions depend on the array length, adding entries would have shifted the problem ontoAsVector128. Reordering toSN_AsVector, SN_AsVector128, SN_AsVector4yields zero unreachable entries, verified by simulating the binary search over the whole table.Measurements
Browser-wasm Release, V8, median ns/op. 3 BEFORE + 3 AFTER runs; medians stable across runs (e.g.
WithElementBEFORE 2.876/2.873/2.875 → AFTER 1.767/1.755/1.775).Vector128<float>.WithElement(2)Matrix4x4.CreateFromQuaternionVector128<byte>.GetElement(11)Vector128<int>.GetElement(3)Quaternion.CreateFromRotationMatrixMatrix4x4.DecomposeVector3.Transform(matrix)Plane.CreateFromVerticesdotnet.native.wasmgrows by 329 bytes (3,158,305 → 3,158,634).One lambda-based benchmark in the first-listed slot measured +6.8%, which I chased rather than dismissed. A dedicated
[MethodImpl(NoInlining)]static loop performing the identical operation measures −7.7% (FloatExtractLoop) and −11.2% (IntExtractLoop), and its IR shows the expected opcode collapse — so the outlier is a first-benchmark warmup artifact, not a regression. Recording it for completeness.Interpreter IR for the extract, before and after
BEFORE — 5 opcodes, including a 16-byte vector copy:
AFTER — 2 opcodes, no vector copy:
The
ldc.i4.ssits immediately before the SIMD instruction that reads it, so the jiterpreter'sget_known_constant_valuerecovers the lane and emits a realextract_lane.Testing
Four new tests in
Vector128Tests.cs. The existingGetElementtests all use loop-variable indices, so none of them exercised the new constant path, and the project had noArgumentOutOfRangeExceptioncoverage at all:Vector128GetElementConstantIndexTest/Vector128WithElementConstantIndexTest— literal indices across all 12 element types, covering sign-extension (-1,-128,short.MinValue), zero-extension (255,65535), and-0.0/NaN/NegativeInfinity. Negative zero is compared by bit pattern viaBitConverter.*Bits, matching the existingTestConstant<T>helper, sinceAssert.Equalwould treat-0.0 == +0.0as equal.Vector128GetElementOutOfRangeTest— out-of-range constant indices must still throw for both APIs.Vector128GetElementVariableOutOfRangeTest—[Theory]over-1,4,int.MaxValue,int.MinValue.[Fact]with literal indices is deliberate here: a[Theory]parameter is not a compile-time constant and would silently stop covering the lowered path.Results:
System.Runtime.Intrinsics.Testson browser-wasm/V8.MONO_VERBOSE_METHODIR dumps that constant-index cases emitsimd_intrins_p_pp/_pppwith zero managed calls, while out-of-range and variable-index cases emit zero SIMD intrinsics and still throw. No "Non-constant lane index" or "out of range" jiterpreter errors across the full 13028-test log.Notes for reviewers
default: return FALSEwith no mutation oftd, and theExtractScalar*/ReplaceScalar*enum values only exist underHOST_BROWSER/HOST_WASI, which is why the new cases carry that guard.packedsimd_alias_methods[]has four unreachable entries (Store,StoreUnsafe,Subtract,SubtractSaturate) becauseSqrtis sorted afterSubtractSaturate. Happy to file it separately.Note
This pull request description was generated by GitHub Copilot.