[wasm][R2R] Fix Vector128 argument alignment in crossgen ArgIterator#131328
Conversation
On wasm, the crossgen R2R<->interpreter thunk and the interpreter/VM ArgIterator disagreed on the stack alignment of Vector128<T> arguments. The crossgen ArgIterator (Wasm32 case) derived value-type alignment from GetFieldAlignment() (InstanceFieldAlignment), which the crossgen type system reports as 8 for Vector128<T>, while the interpreter and runtime 16-align v128 args (getClassAlignmentRequirement). The resulting 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. Add ITypeHandle.RequiresAlign16OnWasm() in the RequiresAlign8 family and use it in the Wasm32 ArgIterator case to 16-align exactly the v128 SIMD types (Vector128<T> / 128-bit Vector<T>, via WasmLowering.IsWasmV128Type), matching the runtime and interpreter. Non-v128 value types keep their prior alignment. Implement the predicate correctly for the cDAC reader by exposing IRuntimeTypeSystem.GetVectorElementSize (delegating to the existing live-metadata GetVectorHFAElementSize detection), so wasm stack-walk argument layout is reconstructed correctly instead of throwing. Fixes dotnet#131299 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 50ddfb2c-b6f8-4847-81e7-44d37d44a175
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
I'm not sure what the best path to take here is? the bug is real and the interpreter and r2r need to agree but I'm not sure how we want to tackle the broader issue of alignment going forward. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Fixes a wasm-specific calling-convention mismatch where Vector128<T> / 128-bit Vector<T> arguments were laid out with 8-byte alignment by crossgen’s ArgIterator, but 16-byte alignment by the runtime/interpreter, causing downstream argument corruption.
Changes:
- Add
ITypeHandle.RequiresAlign16OnWasm()and use it in the Wasm32 ArgIterator path to force 16-byte alignment for wasm v128 SIMD arguments. - Implement the crossgen2
TypeHandle.RequiresAlign16OnWasm()check usingWasmLowering.IsWasmV128Type(madeinternal). - Extend the cDAC RuntimeTypeSystem contract with
GetVectorElementSize(...)and document it indocs/design/datacontracts/RuntimeTypeSystem.md.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/RuntimeTypeSystem_1.cs | Adds GetVectorElementSize implementation backed by existing metadata vector detection. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/Contracts/CallingConvention/CdacTypeHandle.cs | Implements RequiresAlign16OnWasm() for cDAC-backed ArgIterator using the new type-system contract API. |
| src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/Contracts/IRuntimeTypeSystem.cs | Adds GetVectorElementSize to the cDAC contract surface. |
| src/coreclr/tools/Common/JitInterface/WasmLowering.cs | Exposes IsWasmV128Type for reuse by crossgen’s calling convention logic. |
| src/coreclr/tools/Common/CallingConvention/ITypeHandle.cs | Introduces RequiresAlign16OnWasm() to support wasm v128 stack alignment decisions. |
| src/coreclr/tools/Common/CallingConvention/ArgIterator.cs | Uses RequiresAlign16OnWasm() to align v128 value-type arguments to 16 on Wasm32. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TypeHandle.cs | Implements wasm-v128 detection for crossgen’s ITypeHandle. |
| docs/design/datacontracts/RuntimeTypeSystem.md | Updates the contract spec to include GetVectorElementSize. |
Addresses review feedback on dotnet#131328: - Drop the added ITypeHandle.RequiresAlign16OnWasm method; instead make crossgen's TypeHandle.GetFieldAlignment report 16 for wasm v128 types (Vector128<T> / 128-bit Vector<T>) directly. GetFieldAlignment's sole caller is the Wasm32 ArgIterator case, so this has identical containment to the predicate approach with no new interface method and no call-site change. The value is correct on all architectures (InstanceFieldAlignment for Vector128<T> is already 16 on x64/arm64; only crossgen's wasm type system under-reported 8). (max-charlamb) - Rename the cDAC GetVectorHFAElementSize helper to a public GetVectorElementSize contract method instead of adding a thin wrapper; CdacTypeHandle.GetFieldAlignment reports 16 for v128 via it. (max-charlamb) - Clarify that GetVectorElementSize returns 0 for Vector256<T>/Vector512<T>. (adamperlin) The v128 argument alignment behavior is unchanged (byte-identical crossgen output); this only simplifies the shape per maintainer feedback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 50ddfb2c-b6f8-4847-81e7-44d37d44a175
davidwrighton
left a comment
There was a problem hiding this comment.
This feels like the wrong choice on how to apply alignment. I think we should make the field alignment of Vector128<T>/Vector<T> into 16 byte alignment on Wasm. (In both the runtime and crossgen2)
Replaces the ArgIterator/GetFieldAlignment special case with a fix to the underlying field alignment, per review feedback. Every type that lowers to a wasm v128 is 16 bytes and should be 16-byte aligned, but System.Numerics.Vector<T> was left with the 8-byte alignment its metadata layout produces (its two UInt64 fields). Since the wasm thunks encode all v128 types as a single 'V' signature character and RaiseSignature turns that back into whichever v128 type was lowered first, a Vector<T> reaching the cache gave every raised signature 8-byte aligned v128 arguments. That disagreed with the runtime and the interpreter, which 16-align v128 arguments, and the resulting desync corrupted later arguments - producing a spurious NullReferenceException on the vectorized Base64Url.DecodeFromUtf8 path. - crossgen2: the ReadyToRun VectorOfTFieldLayoutAlgorithm now 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. - runtime: MethodTableBuilder::CheckForSystemTypes sets a 16-byte alignment requirement for System.Numerics.Vector<T> on wasm, matching the treatment of Vector128<T> and Int128. - Assert the invariant in CacheV128Type, which sees every v128 type that is lowered. This caught the missing crossgen2 case above. On the cDAC side CdacTypeHandle.GetFieldAlignment now reports 16 for v128 types so wasm argument layout is reconstructed the same way. Other value types still throw NotImplementedException, as before. Fixes dotnet#131299 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 50ddfb2c-b6f8-4847-81e7-44d37d44a175
Remove the cDAC-side changes (GetVectorElementSize contract method and the CdacTypeHandle.GetFieldAlignment v128 special case) from this PR. They re-derived "v128 means 16-byte aligned" by matching type metadata names, which is a partial reimplementation of the runtime's alignment policy that cannot converge on it: the runtime already 16-aligns Int128/UInt128 on wasm too (not vector types), so a Vector-name match can never cover the same set. The faithful cDAC implementation is to read EEClassLayoutInfo's alignment requirement - mirroring CEEInfo::getClassAlignmentRequirementStatic, which is what the runtime wasm ArgIterator uses - which needs its own data descriptor and contract surface and fixes all value types at once. That is left as a separate cDAC-side change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 50ddfb2c-b6f8-4847-81e7-44d37d44a175
Cut the explanatory comments down to the essential invariant; the crossgen2 alignment change and the assert are self-evident from the code. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 50ddfb2c-b6f8-4847-81e7-44d37d44a175
Use `type is DefType defType` in the CacheV128Type assert so an unexpected non-DefType fails via Debug.Assert with the intended message instead of an InvalidCastException from the cast. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 50ddfb2c-b6f8-4847-81e7-44d37d44a175
|
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. |
Explores the follow-up suggested in review on dotnet#131328: give System.Numerics.Vector<T> the same alignment as its matching fixed-size vector on every target, not just wasm. Replaces the per-type, per-architecture alignment switches for Vector64/128/256/512 with a single rule, align = min(size, cap), where cap is the largest vector alignment the target ABI supports: arm 8 (PCS aligns __m128 at 8, no larger vector) arm64 / loongarch64 / riscv64 16 wasm 16 (single v128) x86 / x64 64 (alignment == size) This reproduces every existing intrinsic vector alignment while collapsing the duplicated switches in both the runtime (methodtablebuilder.cpp) and crossgen2 (VectorFieldLayoutAlgorithm.cs). Note the caps are load-bearing: the rule is min(size, cap), not Alignment == Size, or arm64 would over-align Vector512<T> to 64. Vector<T> then picks up the same rule at its ISA-determined size, which CheckIfSIMDAndUpdateSize has already resolved into bmtFP->NumInstanceFieldBytes, and crossgen2 always takes the similar intrinsic vector's alignment (superseding the MATCHING_HARDWARE_VECTOR gate). Measured effect on Vector<T> field alignment (intrinsic vectors unchanged): arm64 size 16 8 -> 16 x64 (AVX2) size 32 8 -> 32 Validation, 0 failures: - System.Runtime.Intrinsics / System.Numerics.Vectors / System.Numerics.Tensors on arm64 and x64: 26,096 tests per arch (52,192 total) - src/tests/JIT/SIMD on arm64: 104/104, and 104/104 again under DOTNET_GCStress=0xC - src/tests/JIT/HardwareIntrinsics/General and JIT/Directed/StructABI on arm64 - ILCompiler.TypeSystem.Tests.TestWrapperAroundVectorTypes (x64 layout oracle) - struct-field/nested-struct offsets, array element stride, and boxing round-trips on both arches; Marshal.SizeOf agrees with managed layout for structs embedding Vector<T>, and explicit FieldOffset placement is honored - Vector<T> by-value argument stress across an interpreter/JIT boundary, passing under both DOTNET_Interpreter=* and the JIT Two notes for a future PR: - No interpreter change is required. x64 Vector<T> at 32-byte alignment exceeds INTERP_STACK_ALIGNMENT (16) and is clamped, which is safe because the interpreter copies value types with memcpy and issues no alignment-sensitive SIMD against interpreter stack slots. Raising the interpreter stack alignment would only cost memory on every frame. - GC heap payloads are not aligned to a type's field alignment (8-byte granular), but this is pre-existing and unrelated: Vector128<T> has been field-aligned to 16 for years and its array and boxed payloads still land at 8 mod 16. Prototype only, not to be merged as-is. On non-wasm targets this changes managed field layout and the R2R cross-module contract, so it requires an R2R version bump. Known gap: NativeAOT has a separate VectorOfTFieldLayoutAlgorithm (src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/VectorOfTFieldLayoutAlgorithm.cs) still using the metadata alignment, which must be updated to match before this could ship. The JIT test trees have not been run on x64. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 528a855a-09f0-4408-be7b-707799f66cc3
Explores the follow-up suggested in review on dotnet#131328: give System.Numerics.Vector<T> the same alignment as its matching fixed-size vector on every target, not just wasm. Replaces the per-type, per-architecture alignment switches for Vector64/128/256/512 with a single rule, align = min(size, cap), where cap is the largest vector alignment the target ABI supports: arm 8 (PCS aligns __m128 at 8, no larger vector) arm64 / loongarch64 / riscv64 16 wasm 16 (single v128) x86 / x64 64 (alignment == size) This reproduces every existing intrinsic vector alignment while collapsing the duplicated switches in the runtime (methodtablebuilder.cpp) and crossgen2 (VectorFieldLayoutAlgorithm.cs). Note the caps are load-bearing: the rule is min(size, cap), not Alignment == Size, or arm64 would over-align Vector512<T> to 64. The alignment also cannot fall out of a natural field walk, because the Vector128 -> Vector64 tower bottoms out at a single UInt64, so max-field-alignment yields 8 for every one of these types on every target. Vector<T> then picks up the same rule at its ISA-determined size in all three engines: the runtime (CheckIfSIMDAndUpdateSize has already resolved the size into bmtFP->NumInstanceFieldBytes), crossgen2 (which takes the similar intrinsic vector's alignment, superseding the MATCHING_HARDWARE_VECTOR gate), and NativeAOT (whose separate VectorOfTFieldLayoutAlgorithm previously kept the metadata alignment). Measured effect on Vector<T> field alignment (intrinsic vectors unchanged): arm64 size 16 8 -> 16 x64 (AVX2) size 32 8 -> 32 Validation, 0 failures: - System.Runtime.Intrinsics / System.Numerics.Vectors / System.Numerics.Tensors on arm64 and x64: 26,096 tests per arch (52,192 total) - src/tests/JIT/SIMD on arm64: 104/104, and 104/104 again under DOTNET_GCStress=0xC - src/tests/JIT/HardwareIntrinsics/General and JIT/Directed/StructABI on arm64 - ILCompiler.TypeSystem.Tests.TestWrapperAroundVectorTypes (x64 layout oracle) - struct-field/nested-struct offsets, array element stride, and boxing round-trips on both arches; Marshal.SizeOf agrees with managed layout for structs embedding Vector<T>, and explicit FieldOffset placement is honored - Vector<T> by-value argument stress across an interpreter/JIT boundary, passing under both DOTNET_Interpreter=* and the JIT Two notes for a future PR: - No interpreter change is required. x64 Vector<T> at 32-byte alignment exceeds INTERP_STACK_ALIGNMENT (16) and is clamped, which is safe because the interpreter copies value types with memcpy and issues no alignment-sensitive SIMD against interpreter stack slots. Raising the interpreter stack alignment would only cost memory on every frame. - GC heap payloads are not aligned to a type's field alignment (8-byte granular), but this is pre-existing and unrelated: Vector128<T> has been field-aligned to 16 for years and its array and boxed payloads still land at 8 mod 16. Prototype only, not to be merged as-is. On non-wasm targets this changes managed field layout and the R2R cross-module contract, so it requires an R2R version bump. The JIT test trees have not yet been run on x64. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 528a855a-09f0-4408-be7b-707799f66cc3
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 spuriousNullReferenceExceptionon the vectorizedBase64Url.DecodeFromUtf8path.Root cause
Every type that lowers to a wasm
v128is 16 bytes and should be 16-byte aligned.Vector128<T>already is, butSystem.Numerics.Vector<T>was left with the 8-byte alignment its metadata layout produces (from its twoUInt64fields).That leaks into every thunk because the wasm thunks don't carry real types:
LowerSignatureencodes all v128 types as a single'V'signature character, andRaiseSignatureturns'V'back into whichever v128 type happened to be lowered first (CompilerTypeSystemContext.CachedV128Type). When that cached type was aVector<T>, every raised signature got 8-byte aligned v128 arguments, disagreeing with the runtime and interpreter, which 16-align them.Fix
VectorOfTFieldLayoutAlgorithmtakes the field alignment from the matching intrinsic vector (Vector128<T>) on wasm, whereVector<T>does follow the intrinsic vector calling convention. Other targets are unchanged and keep the metadata alignment, matching the existingMATCHING_HARDWARE_VECTORnote.MethodTableBuilder::CheckForSystemTypessets a 16-byte alignment requirement forSystem.Numerics.Vector<T>on wasm, alongside the existing handling forVector128<T>andInt128.CacheV128Typeasserts 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
System.Private.CoreLibfor wasm passes the new invariant assert. Without the crossgen2 fix it fires onSystem.Numerics.Vector\1`, 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).Microsoft.Bcl.Memory.Testswith 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 noBase64Helper.DecodeFromNullReferenceExceptionin any run. Before the fix this suite had 12Base64Urlfailures, allNullReferenceExceptioninBase64Helper.DecodeFrom; the failure was also observed under a WASI debugger, paused at thecall_indirectinBase64Helper.DecodeFrom<Base64UrlDecoderByte, UInt8>across the thunk boundary.Microsoft.Bcl.Numerics.Tests882/882, no regressions from the runtime alignment change.Note: that SIMD-enabled R2R run requires the
PackedSimd.Shufflelowering from #130991, which is not inmainyet — without it, composite images built from currentmainfail to load becausePackedSimd.Shuffleemits an out-of-rangei8x16.shufflemask. 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 realEEClassLayoutInfoalignment (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.