[RyuJIT Wasm] Handle all codegen cases for PackedSimd.Shuffle#130991
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 5 pipeline(s). 10 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 this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Adds/extends WebAssembly SIMD intrinsic lowering and codegen in CoreCLR RyuJIT, primarily to enable PackedSimd.Shuffle codegen and to move lane load/store intrinsics into the table-driven pipeline.
Changes:
- Add
LowerHWIntrinsicNativeShuffleto supportNI_PackedSimd_Shuffle(emit native shuffle for constant masks; rewrite non-constant masks to swizzle+swizzle+or). - Introduce
HW_Flag_HasImmediateOperandfor WASM and use it to drive lowering/codegen decisions (lane immediates, jump-table fallback, containment). - Reclassify/enable WASM lane load/store intrinsics as
HW_Category_MemoryLoad/MemoryStoreand update codegen +OperIsMemoryLoad/Storeto recognize WASM.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/lowerwasm.cpp | Adds shuffle lowering; uses HasImmediateOperand to drive immediate-lane lowering and containment. |
| src/coreclr/jit/lower.h | Declares the new WASM lowering helper LowerHWIntrinsicNativeShuffle. |
| src/coreclr/jit/hwintrinsicwasm.cpp | Removes special-import handling for some lane-load intrinsics now handled table-driven; keeps immediate upper-bound logic for lane ops. |
| src/coreclr/jit/hwintrinsiclistwasm.h | Reclassifies lane load/store intrinsics to memory categories; adds immediate-operand flags for relevant intrinsics. |
| src/coreclr/jit/hwintrinsiccodegenwasm.cpp | Emits v128 immediates for shuffle; adds table-driven codegen for WASM memory load/store categories and jump-table support for lane memops. |
| src/coreclr/jit/hwintrinsic.h | Adds WASM HW_Flag_HasImmediateOperand, updates HasImmediateOperand, and adds shuffle imm-op position + immediate vector accessor. |
| src/coreclr/jit/gentree.cpp | Extends OperIsMemoryLoad/OperIsMemoryStore to include WASM. |
| src/coreclr/jit/codegenwasm.cpp | Removes NYI guard for SIMD16 store-indirect to allow emitting stores. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…r clarity since it is a special case
…131328) On wasm, the crossgen R2R↔interpreter thunks and the interpreter/VM ArgIterator disagreed on the stack alignment of `Vector128<T>` arguments. The 8-byte desync accumulated per v128 argument, corrupting later arguments (e.g. the trailing generic-context byref) and producing a spurious `NullReferenceException` on the vectorized `Base64Url.DecodeFromUtf8` path. ### Root cause Every type that lowers to a wasm `v128` is 16 bytes and should be 16-byte aligned. `Vector128<T>` already is, but `System.Numerics.Vector<T>` was left with the 8-byte alignment its metadata layout produces (from its two `UInt64` fields). That leaks into every thunk because the wasm thunks don't carry real types: `LowerSignature` encodes all v128 types as a single `'V'` signature character, and `RaiseSignature` turns `'V'` back into whichever v128 type happened to be lowered first (`CompilerTypeSystemContext.CachedV128Type`). When that cached type was a `Vector<T>`, every raised signature got 8-byte aligned v128 arguments, disagreeing with the runtime and interpreter, which 16-align them. ### Fix - **crossgen2**: the ReadyToRun `VectorOfTFieldLayoutAlgorithm` takes the field alignment from the matching intrinsic vector (`Vector128<T>`) on wasm, where `Vector<T>` does follow the intrinsic vector calling convention. Other targets are unchanged and keep the metadata alignment, matching the existing `MATCHING_HARDWARE_VECTOR` note. - **runtime**: `MethodTableBuilder::CheckForSystemTypes` sets a 16-byte alignment requirement for `System.Numerics.Vector<T>` on wasm, alongside the existing handling for `Vector128<T>` and `Int128`. - **invariant**: `CacheV128Type` asserts that every v128 type it sees is 16-byte aligned, since a v128 type with a smaller alignment would silently change raised signatures depending on lowering order. This assert is what caught the missing crossgen2 case above. ### Validation - A Debug crossgen2 compiling all of `System.Private.CoreLib` for wasm passes the new invariant assert. Without the crossgen2 fix it fires on `System.Numerics.Vector\`1<uint8>`, which is how the missing case was found. - `WasmR2RToInterpreterThunk(VVVp)` stores its v128 arguments at frame+16 and frame+32 (16-byte aligned, 16-byte stride). - Browser R2R, `Microsoft.Bcl.Memory.Tests` with SIMD fully R2R-compiled (`JitWasmSimdNyiToR2RUnsupported=0`, the configuration that actually exercises v128 arguments): 550/550 across three runs with R2R on, 550/550 with R2R off, and no `Base64Helper.DecodeFrom` `NullReferenceException` in any run. Before the fix this suite had 12 `Base64Url` failures, all `NullReferenceException` in `Base64Helper.DecodeFrom`; the failure was also observed under a WASI debugger, paused at the `call_indirect` in `Base64Helper.DecodeFrom<Base64UrlDecoderByte, UInt8>` across the thunk boundary. - `Microsoft.Bcl.Numerics.Tests` 882/882, no regressions from the runtime alignment change. Note: that SIMD-enabled R2R run requires the `PackedSimd.Shuffle` lowering from #130991, which is not in `main` yet — without it, composite images built from current `main` fail to load because `PackedSimd.Shuffle` emits an out-of-range `i8x16.shuffle` mask. The runs above were done on a branch that includes it. This is unrelated to argument alignment and affects builds with and without this change identically. An automated regression test is deferred: wasm R2R is not CI-runnable yet, and no existing test project reaches crossgen's `ArgIterator<TypeHandle>` (the cDAC ARGITER stress harness runs x64/x86/ARM under corerun and never enters the Wasm32 branch). Deferred regression test tracked in #131339. A cDAC-side follow-up that reads the real `EEClassLayoutInfo` alignment (fixing all value types, not just v128) is tracked separately in #131343. Fixes #131299 > [!NOTE] > Parts of this pull request description were generated with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 50ddfb2c-b6f8-4847-81e7-44d37d44a175
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/tests/JIT/HardwareIntrinsics/Wasm/PackedSimd/PackedSimdTests.cs:1085
- This test was disabled by commenting it out, which drops coverage for null-check behavior of these load/store intrinsics. If it’s flaky or platform-dependent, it should be guarded with an appropriate conditional attribute; otherwise it should remain enabled.
// [Fact]
// public static unsafe void LoadStoreNullCheckTest()
// {
// Assert.Throws<NullReferenceException>(() => LoadScalarAndSplatVector128(null));
// Assert.Throws<NullReferenceException>(() => LoadScalarVector128(null));
// Assert.Throws<NullReferenceException>(() => LoadWideningVector128(null));
// Assert.Throws<NullReferenceException>(() => LoadScalarAndInsert(null, 2));
// Assert.Throws<NullReferenceException>(() => StoreSelectedScalar(null, 2));
// }
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/coreclr/jit/lowerwasm.cpp:1402
- This comment uses an incorrect exception name (ArrayIndexOutOfBoundsException) and also describes the check as GreaterThanAny(mask, 31), while the actual behavior is a ">= 32" range check that throws the runtime's range-check exception. Please align the comment with the actual condition and exception type used.
// This is roughly equivalent to the following C#:
// ...
// if (GreaterThanAny(originalMask, 31)) { throw new ArrayIndexOutOfBoundsException(); }
// tmp1 = PackedSimd.Swizzle(op1, originalMask);
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/coreclr/jit/lowerwasm.cpp:1381
- This comment block says the bounds-check uses
CNS_INT int 31, but the actual code below buildsgtNewIconNode(32)and codegen checksmask >= bound(so32is the correct bound). Updating the comment will avoid confusion when debugging LIR dumps.
// m0 = * LCL_VAR originalMaskTmp
// b0 = * CNS_INT int 32
// /--* m0 simd
// +--* b0 int
// GT_BOUNDS_CHECK RNG_CHK_FAIL
src/coreclr/jit/lowerwasm.cpp:1403
- The pseudo-C# uses
ArrayIndexOutOfBoundsException, which isn’t a .NET exception type, and it references a bound of31while the implementation uses>= 32. Since this comment is describing the exact semantics of the insertedGT_BOUNDS_CHECK, it should match the code more directly.
// This is roughly equivalent to the following C#:
// ...
// if (GreaterThanAny(originalMask, 31)) { throw new ArrayIndexOutOfBoundsException(); }
// tmp1 = PackedSimd.Swizzle(op1, originalMask);
// tmp2 = PackedSimd.Swizzle(op2, PackedSimd.Subtract(originalMask, PackedSimd.Splat(0x10)));
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/coreclr/jit/codegenwasm.cpp:2287
- genRangeCheck treats any SIMD-typed GT_BOUNDS_CHECK index as a byte-vector mask and emits i8x16.* ops unconditionally. That assumption is only valid for the new shuffle-mask bounds check (SCK_ARG_RNG_EXCPN) and could silently miscompile any future bounds checks that use SIMD indices with non-byte lane semantics. Consider restricting the SIMD path to the specific throw kind / expected shape, and assert the length operand is an int scalar.
#ifdef FEATURE_SIMD
if (varTypeIsSIMD(boundsCheck->GetIndex()->TypeGet()))
{
GetEmitter()->emitIns(INS_i8x16_splat);
GetEmitter()->emitIns(INS_i8x16_ge_u);
GetEmitter()->emitIns(INS_v128_any_true);
}
else
#endif
{
GetEmitter()->emitIns(INS_I_ge_u);
}
…otnet#131328) On wasm, the crossgen R2R↔interpreter thunks and the interpreter/VM ArgIterator disagreed on the stack alignment of `Vector128<T>` arguments. The 8-byte desync accumulated per v128 argument, corrupting later arguments (e.g. the trailing generic-context byref) and producing a spurious `NullReferenceException` on the vectorized `Base64Url.DecodeFromUtf8` path. ### Root cause Every type that lowers to a wasm `v128` is 16 bytes and should be 16-byte aligned. `Vector128<T>` already is, but `System.Numerics.Vector<T>` was left with the 8-byte alignment its metadata layout produces (from its two `UInt64` fields). That leaks into every thunk because the wasm thunks don't carry real types: `LowerSignature` encodes all v128 types as a single `'V'` signature character, and `RaiseSignature` turns `'V'` back into whichever v128 type happened to be lowered first (`CompilerTypeSystemContext.CachedV128Type`). When that cached type was a `Vector<T>`, every raised signature got 8-byte aligned v128 arguments, disagreeing with the runtime and interpreter, which 16-align them. ### Fix - **crossgen2**: the ReadyToRun `VectorOfTFieldLayoutAlgorithm` takes the field alignment from the matching intrinsic vector (`Vector128<T>`) on wasm, where `Vector<T>` does follow the intrinsic vector calling convention. Other targets are unchanged and keep the metadata alignment, matching the existing `MATCHING_HARDWARE_VECTOR` note. - **runtime**: `MethodTableBuilder::CheckForSystemTypes` sets a 16-byte alignment requirement for `System.Numerics.Vector<T>` on wasm, alongside the existing handling for `Vector128<T>` and `Int128`. - **invariant**: `CacheV128Type` asserts that every v128 type it sees is 16-byte aligned, since a v128 type with a smaller alignment would silently change raised signatures depending on lowering order. This assert is what caught the missing crossgen2 case above. ### Validation - A Debug crossgen2 compiling all of `System.Private.CoreLib` for wasm passes the new invariant assert. Without the crossgen2 fix it fires on `System.Numerics.Vector\`1<uint8>`, which is how the missing case was found. - `WasmR2RToInterpreterThunk(VVVp)` stores its v128 arguments at frame+16 and frame+32 (16-byte aligned, 16-byte stride). - Browser R2R, `Microsoft.Bcl.Memory.Tests` with SIMD fully R2R-compiled (`JitWasmSimdNyiToR2RUnsupported=0`, the configuration that actually exercises v128 arguments): 550/550 across three runs with R2R on, 550/550 with R2R off, and no `Base64Helper.DecodeFrom` `NullReferenceException` in any run. Before the fix this suite had 12 `Base64Url` failures, all `NullReferenceException` in `Base64Helper.DecodeFrom`; the failure was also observed under a WASI debugger, paused at the `call_indirect` in `Base64Helper.DecodeFrom<Base64UrlDecoderByte, UInt8>` across the thunk boundary. - `Microsoft.Bcl.Numerics.Tests` 882/882, no regressions from the runtime alignment change. Note: that SIMD-enabled R2R run requires the `PackedSimd.Shuffle` lowering from dotnet#130991, which is not in `main` yet — without it, composite images built from current `main` fail to load because `PackedSimd.Shuffle` emits an out-of-range `i8x16.shuffle` mask. The runs above were done on a branch that includes it. This is unrelated to argument alignment and affects builds with and without this change identically. An automated regression test is deferred: wasm R2R is not CI-runnable yet, and no existing test project reaches crossgen's `ArgIterator<TypeHandle>` (the cDAC ARGITER stress harness runs x64/x86/ARM under corerun and never enters the Wasm32 branch). Deferred regression test tracked in dotnet#131339. A cDAC-side follow-up that reads the real `EEClassLayoutInfo` alignment (fixing all value types, not just v128) is tracked separately in dotnet#131343. Fixes dotnet#131299 > [!NOTE] > Parts of this pull request description were generated with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 50ddfb2c-b6f8-4847-81e7-44d37d44a175
This is fallback codgen in lowering for if we encounter
PackedSimd.Shuffle(v1, v2, non_const_mask). We transform this into:This works because the range for the mask in shuffle should be
[0, 31], whereas swizzle simply writes zero for any index not in[0, 15]. So, the original mask selects elements from the first vector, and the modifiedmask - 16truncates any indices which correspond to the first vector to be out of range, allowing for selection from the second vector.PackedSimd.Shuffle is internal to System.Private.CoreLib, so it's not part of the public API surface but we do need to handle it to build System.Private.CoreLib.