[wasm][coreclr] Fix reflection with large valuetypes #123759
Conversation
|
I am also writing down my thoughts on #120791 and will add them soon |
|
Tagging subscribers to this area: @mangod9 |
There was a problem hiding this comment.
Pull request overview
This PR updates the WASM/CoreCLR interpreter calling convention support so reflection invoke can correctly pass/return large value types (notably SIMD vectors), including adjusting the special-argument ordering (retbuf vs this) and improving stack layout alignment.
Changes:
- Adjust WASM reflection invoke/call-descriptor plumbing to account for retbuf presence and interpreter stack-slot sizing.
- Update ArgIterator WASM logic for special-arg offsets and value-type alignment handling.
- Fix a test failure message to correctly reference
Vector128<T>.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/JIT/Directed/VectorABI/VectorMgdMgdArray.cs | Updates failure message text to reflect Vector128<T> checks. |
| src/coreclr/vm/wasm/cgencpu.h | Refines return-type size macros used by calling convention logic. |
| src/coreclr/vm/wasm/calldescrworkerwasm.cpp | Adjusts interpreter call argument pointer/size handling when a retbuf is present. |
| src/coreclr/vm/reflectioninvocation.cpp | Updates stack-slot counting and records retbuf presence for WASM reflection calls. |
| src/coreclr/vm/frames.cpp | Recomputes this offset via ArgIterator instance (signature-aware). |
| src/coreclr/vm/callingconvention.h | Makes GetThisOffset instance-based; updates WASM special-arg ordering and adds valuetype alignment logic. |
| src/coreclr/vm/callhelpers.h | Adds hasRetBuff field (WASM-only) to CallDescrData. |
| src/coreclr/vm/callhelpers.cpp | Initializes hasRetBuff for WASM call paths and adds a debug assert. |
| #define SPECIAL_ARG_ADDR(pos) (((int8_t*)pCallDescrData->pSrc) + ((pos)*INTERP_STACK_SLOT_SIZE)) | ||
|
|
There was a problem hiding this comment.
SPECIAL_ARG_ADDR is a macro that implicitly depends on a local name (pCallDescrData). This is fragile (easy to misuse elsewhere, no type safety, and can lead to surprising captures). Consider replacing it with a small static inline helper that takes CallDescrData* and the slot index.
| if (pCallDescrData->hasRetBuff) { | ||
| argsSize -= INTERP_STACK_SLOT_SIZE; | ||
| } |
There was a problem hiding this comment.
Brace placement in this file consistently uses the opening brace on the next line (e.g., the targetIp == NULL block). The new if (pCallDescrData->hasRetBuff) { deviates from that style; please match the existing formatting for consistency.
There was a problem hiding this comment.
Or feel free to remove the braces entirely if the branch case is a single statement.
| ret += offsetof(ArgumentRegisters, ECX); | ||
| #endif | ||
| #elif defined(TARGET_WASM) | ||
| if (this->HasRetBuffArg() && IsRetBuffPassedAsFirstArg()) |
There was a problem hiding this comment.
There is a member IsRetBuffPassedAsFirstArg() and a stand alone IsRetBuffPassedAsFirstArg(). It isn't obvious to me which one we should be relying on. It looks like there are multiple architectural designs here and it would be great if we could clean them up. For WASM specifically, its as through the RetBuff is always passed as the first arg, is that true?
| if (align < INTERP_STACK_SLOT_SIZE) | ||
| align = INTERP_STACK_SLOT_SIZE; | ||
|
|
||
| if (align > INTERP_STACK_ALIGNMENT) | ||
| align = INTERP_STACK_ALIGNMENT; |
There was a problem hiding this comment.
I would like to see comments on this logic. It looks as if the SLOT_SIZE and STACK_ALIGNMENT are both constants, can't we simplify this? In the sense of std::min(align, INTERP_STACK_ALIGNMENT)?
| TADDR TransitionFrame::GetAddrOfThis() | ||
| { | ||
| WRAPPER_NO_CONTRACT; | ||
| return GetTransitionBlock() + ArgIterator::GetThisOffset(); |
There was a problem hiding this comment.
This makes wasm more different from all other architectures. this is in a fixed location on all other architectures. In the past, we had some cases where it was not the case and we got rid of them since keep this in a fixed location everywhere makes things simpler overall.
Can we keep it like that for wasm too (ie revert this change)?
Instead, move the return bufffer pointer to either dedicated location/register (like it is on arm64) or to be after the this pointer (like it is on x64).
| MethodDesc *pFunction = GetFunction(); | ||
| MetaSig msig(pFunction); | ||
| ArgIterator argit (&msig); | ||
| return GetTransitionBlock() + argit.GetThisOffset(); |
There was a problem hiding this comment.
The GetAddrOfThis() API can be called from various locations. It might be worth caching this value somewhere. The previous implementation had far less state than what is being done here.
3 participants
Implement return of large valuetypes from reflection calls and also large valuetypes arguments.
Change the order of
thisandretbufarguments. This goes toward the #120791 and the new calling convention. It also helps to keep the interpreter calls use simple memory copy.JIT/Directed/Directed_3 runtime tests were used as repro.
Fixes #122922