[wasm] Pass Int128, Decimal128 and wide vectors by value - #131492
Conversation
|
Tagging @dotnet/jit-contrib for JIT-EE GUID update |
|
Azure Pipelines: Successfully started running 7 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates the wasm calling convention/signature encoding so certain “wide” intrinsic value types (e.g., Int128/UInt128, Vector256, Vector512, Decimal128) are passed by value across multiple wasm parameters instead of being encoded as ambiguous S<N> by-ref structs. The change spans the runtime encoder, crossgen2 lowering/raising, R2R thunk generation, and the JIT’s ABI classification so all components agree on the same signature identity and frame layout.
Changes:
- Add a multi-slot signature token form (
l2,V2,V4) and use it for qualifying intrinsic wide types when passed as parameters; keep returns asS<N>(retbuf). - Update runtime and crossgen2 signature lowering/raising and R2R thunk emitters to correctly marshal multi-slot arguments.
- Add/extend unit tests and update the ready-to-run format documentation; reject multi-slot types in WasmAppBuilder interop signatures.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tasks/WasmAppBuilder/coreclr/SignatureMapper.cs | Reject multi-slot ABI types for interop signatures; parse multi-slot tokens. |
| src/coreclr/vm/wasm/helpers.cpp | Runtime signature encoding supports multi-slot parameter tokens and preserves return behavior. |
| src/coreclr/tools/Common/JitInterface/WasmLowering.cs | Crossgen2 lowering/raising now classifies and encodes multi-slot parameters and separates return-struct caching. |
| src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs | Exposes multi-slot slot-type lowering to the JIT via getWasmLowering. |
| src/coreclr/tools/Common/Compiler/CompilerTypeSystemContext.Wasm.cs | Adds elevated-type resolution and a separate cache for return structs to keep raising stable/unambiguous. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/JitInterface/WasmLowering.ReadyToRun.cs | Ensures multi-slot types are treated as by-value (not byref) in R2R paths. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmR2RToInterpreterThunkNode.cs | Stores multi-slot wasm locals into the interpreter argument area slot-by-slot. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmInterpreterToR2RThunkNode.cs | Loads multi-slot arguments from interpreter arg area and forwards each slot as a wasm parameter. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/WasmImportThunk.cs | Stashes/restores multi-slot arguments as multiple locals during import thunk transitions. |
| src/coreclr/tools/aot/ILCompiler.ReadyToRun.Tests/WasmArgumentLayoutTests.cs | Adds coverage for multi-slot encoding, raising stability, cache poisoning avoidance, and Decimal128 shape. |
| src/coreclr/jit/targetwasm.cpp | Splits struct args into multiple wasm “register” segments when size exceeds the lowered wasm value width. |
| src/coreclr/jit/lower.cpp | Adjusts FieldList/PutArg marking logic for targets without fixed register sets (wasm). |
| src/coreclr/jit/compiler.cpp | Forces multi-slot returns to use retbuf (byref) while allowing multi-slot passing. |
| src/coreclr/jit/codegenwasm.cpp / src/coreclr/jit/codegen.h | Consumes FIELD_LIST args per-field for correct liveness on wasm (no PUTARG nodes). |
| src/coreclr/inc/jiteeversionguid.h | Bumps JIT/EE interface GUID. |
| docs/design/coreclr/botr/readytorun-format.md | Documents the multi-slot token grammar and examples. |
13d4bc8 to
7bbf6e8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/coreclr/tools/Common/JitInterface/WasmLowering.cs:689
sigBuilder.Append(slotCount);will append multi-digit numbers (e.g., 10) if slotCount ever exceeds 9, which would make the signature ambiguous/misparsed (RaiseSignature only consumes a single digit). Prefer emitting a single digit char and assert the supported range so malformed signatures fail fast.
if (TryGetMultiSegmentLayout(paramType, out WasmValueType slotType, out int slotCount))
{
// Passed by value across several wasm parameters, matching the wasm C ABI.
// Spelled '<slot><elevation>'; the elevation factor equals the slot count
// for every type in the wasm ABI today, and the encoding cannot express
// them differing. See readytorun-format.md.
sigBuilder.Append(WasmValueTypeToSigChar(slotType));
sigBuilder.Append(slotCount);
for (int slot = 0; slot < slotCount; slot++)
{
result.Add(slotType);
These types were passed as a single i32 pointer under the 'S<N>' encoding, which is
both the wrong convention and ambiguous. Measured against clang for wasm32, an
__int128 is passed as 2 x i64 by value, a 256-bit vector as 2 x v128 and a 512-bit
vector as 4 x v128. All three are returned through a hidden buffer, which is what
'S<N>' already does, so returns are left alone.
The ambiguity is the more immediate problem. 'S<N>' resolves through a size-keyed
first-wins struct cache, so Int128 and Guid both spell 'S16' and share a thunk whose
frame layout only fits whichever the cache happened to see first. That reproduces as
miscompiles under R2R.
Encode such a parameter as the slot character followed by the factor its alignment is
elevated above the slot's natural alignment -- 'l2', 'V2', 'V4' -- which stays
unambiguous when mixed, as in 'll2VV4'. An elevation factor differing from the slot
count is not expressible; no type today needs one.
Classify structurally rather than by name: intrinsic, since the wasm C ABI passes an
ordinary aggregate indirectly and struct { long a, b; } must stay indirect; aligned to
its own size, which excludes Vector2/3/4; and wider than one slot, which excludes v128
and Vector64<T> without naming them. Decimal128 is picked up by that rule without
being mentioned anywhere. Alignment is unchanged throughout -- it already matches
clang.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 8c87b08f-c9ef-4fcd-b506-8790ce76331a
WasmAppBuilder encodes the signatures behind the PInvoke and InternalCall tables, where a type the wasm ABI splits across several by-value slots would need one signature token to map to several native parameters. Teaching the thunk generator that is a larger question than this change, and no interop signature uses one of these types today, so report WASM0068 rather than encode it. The parser still understands the encoding, since signatures it reads can carry one. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8c87b08f-c9ef-4fcd-b506-8790ce76331a
7bbf6e8 to
a436442
Compare
|
cc @dotnet/wasm-contrib |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/tasks/WasmAppBuilder/coreclr/SignatureMapper.cs:186
- The wrapper-unwrapping heuristic in IsMultiSlotType assumes “a single field fills its struct unless the struct declares a size”, but that’s not true for explicit-layout wrappers with a non-zero FieldOffset. Those can be larger than the field without setting StructLayoutAttribute.Size, and unwrapping them here can cause over-rejection (treating an indirect struct as multi-slot). Consider checking for explicit non-zero field offsets and avoiding unwrapping in that case (still keeping the explicit Size check).
// Only unwrap a wrapper its field fills exactly. Sizes cannot be measured here: these
// types come from a MetadataLoadContext, where Marshal.SizeOf always throws. A single
// field fills its struct unless the struct declares a size of its own.
FieldInfo[] fields = t.GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
if (fields.Length != 1 || t.StructLayoutAttribute is { Size: not 0 })
{
return false;
}
src/coreclr/vm/wasm/helpers.cpp:1020
- ConvertType::ToSlotsI64 is now also used for Decimal128 (it shares the
l2encoding), but the comment only mentions Int128/UInt128. Updating the comment would help keep the three encoders’ intent aligned while debugging ABI classification.
ToSlotsI64, // Passed by value as several i64 slots (Int128/UInt128)
ToSlotsV128, // Passed by value as several v128 slots (Vector256<T>, Vector512<T>)
|
Azure Pipelines: Successfully started running 7 pipeline(s). 9 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
AndyAyersMS
left a comment
There was a problem hiding this comment.
JIT changes look good, the rest looks reasonable, but I'll defer to @davidwrighton for those.
| | Encoding | Slots | Alignment | Type | | ||
| |---|---|---|---| | ||
| | `l2` | 2 x `i64` | 16 (elevated 2x) | `Int128`, `UInt128`, `Decimal128` | | ||
| | `V2` | 2 x `v128` | 32 (elevated 2x) | `Vector256<T>` | |
There was a problem hiding this comment.
The JIT is currently not able to support any alignment beyond 16 bytes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/coreclr/tools/Common/JitInterface/WasmLowering.cs:688
- The multi-slot encoding is documented (and parsed in RaiseSignature / WasmAppBuilder) as a single digit elevation factor, but here
sigBuilder.Append(slotCount)will emit multiple characters whenslotCount >= 10, producing a signature string that RaiseSignature cannot round-trip (it only consumes one digit) and that would be ambiguous in the overall grammar. Since the encoding can only represent a single digit, this should append a single digit char and assert the supported range (matching the runtime encoder’sslotCount <= 9invariant).
// Passed by value across several wasm parameters, matching the wasm C ABI.
// Spelled '<slot><elevation>'; the elevation factor equals the slot count
// for every type in the wasm ABI today, and the encoding cannot express
// them differing. See readytorun-format.md.
sigBuilder.Append(WasmValueTypeToSigChar(slotType));
sigBuilder.Append(slotCount);
for (int slot = 0; slot < slotCount; slot++)
{
result.Add(slotType);
}
src/tasks/WasmAppBuilder/coreclr/SignatureMapper.cs:186
- The wrapper-unwrapping heuristic (
fields.Length == 1and no explicitStructLayoutAttribute.Size) can misclassify explicit-layout wrappers. For example, an explicit-layout struct with a single field at a non-zeroFieldOffset(or other layout-induced padding) does not satisfy the “field fills struct exactly” rule that the runtime / crossgen2 use (they both compare the field size to the struct size). Since WasmAppBuilder can’t measure size forMetadataLoadContexttypes, it should conservatively refuse to unwrap explicit-layout / FieldOffset wrappers to avoid rejecting otherwise-valid interop signatures or incorrectly tagging them as multi-slot.
// Only unwrap a wrapper its field fills exactly. Sizes cannot be measured here: these
// types come from a MetadataLoadContext, where Marshal.SizeOf always throws. A single
// field fills its struct unless the struct declares a size of its own.
FieldInfo[] fields = t.GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance);
if (fields.Length != 1 || t.StructLayoutAttribute is { Size: not 0 })
{
return false;
}
Problem
Int128/UInt128,Decimal128,Vector256<T>andVector512<T>are passed on wasm as a singlei32pointer under theS<N>signature encoding. That is both the wrong calling convention and ambiguous.Wrong convention. Measured against clang for wasm32 (the in-tree wasi-sdk):
__int128i64by valuev128by valuev128by valueThe returns are unaffected by
-mmultivalue, andS<N>already returns through a hidden buffer, so returns are left alone. Alignment already matches clang (16/32/64) and is unchanged by this PR.Ambiguous.
S<N>resolves through a size-keyed, first-wins struct cache, soInt128andGuidboth spellS16and share a thunk whose frame layout only fits whichever type the cache happened to see first. This reproduces as miscompiles under R2R today:Int128.EqualsreturnsFalseandCompareToreturns ±1 for values that are equal.Change
Encode such a parameter as the slot character followed by the factor its alignment is elevated above the slot's natural alignment:
llongl2Int128,UInt128,Decimal128VVector128<T>,Vector<T>V2Vector256<T>V4Vector512<T>This stays unambiguous when mixed:
ll2VV4isi64,Int128,Vector128,Vector512. An elevation factor that differs from the slot count is not expressible; no type today needs one.Classification is structural rather than by name — intrinsic, aligned to its own size, and wider than one slot. Each condition is load-bearing:
struct { long a, b; }decomposes exactly likeInt128but must stay indirect.Vector2/3/4, which are intrinsic but 4-aligned. Alignment alone would not do, sinceStructLayout(Pack)can manufacture it.v128andVector64<T>without naming them. The threshold must be the slot's own width rather than a fixed number, sinceInt128needs two slots at the same 16 bytes av128fills with one.Decimal128is picked up by that rule without being named anywhere.Three encoders have to agree exactly, since the signature string is a thunk's identity: crossgen2 (
WasmLowering), the runtime (vm/wasm/helpers.cpp), and WasmAppBuilder (SignatureMapper).An encoding records layout, not identity, so raising resolves a stand-in —
l2always raises toInt128. That is sound because a thunk is keyed by its signature string and its frame layout derives from nothing else, which the tests pin.Interop
WasmAppBuilder reports
WASM0068for these types rather than encoding them. Supporting them means teaching the thunk generator that one signature token can map to several native parameters, which is a larger design question; noPInvokeorInternalCallsignature uses one today.Validation
Browser wasm, Release and Checked, both
DOTNET_ReadyToRun=0and=1:Int128equality/comparison reproVector256/Vector512/Vector64reproMicrosoft.Bcl.MemorySystem.Runtime.CompilerServices.UnsafeWasmArgumentLayoutTests--verify-type-and-field-layoutEach repro was confirmed to fail without the change before being trusted as a pass, and the runtime encoder was confirmed live by perturbation (emitting the wrong slot char breaks R2R=1 with a function signature mismatch).
Notes
src/coreclr, then the interop rejection undersrc/tasks.Note
This pull request was prepared with GitHub Copilot.