[Wasm R2R] Retype integer constant struct returns to the native return type#129102
[Wasm R2R] Retype integer constant struct returns to the native return type#129102lewing wants to merge 4 commits into
Conversation
When `LowerRetStruct` retypes a struct return to a primitive `nativeReturnType` and the return value is a `GT_CNS_INT`, the integer branch left the constant as `TYP_INT` even when the native return type is wider (e.g. `TYP_LONG`). On register targets this is harmless because integer registers are untyped, but wasm has a typed value stack, so codegen emitted `i32.const 0` for an `i64`-typed return. This surfaced when validating the R2R-compiled `System.Private.CoreLib` for wasm: `System.Runtime.Intrinsics.Vector64<byte>.get_Zero` returns a zero-initialized 8-byte struct, which the wasm ABI lowers to an `i64` return. The emitted body ended with `i32.const 0`, and wasmtime rejected the module with "type mismatch: expected i64, found i32". Retype the integer constant to the native return type under `TARGET_WASM` so codegen emits an `i64.const`. The fix applies to both the browser and wasi wasm targets, which share the same JIT. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
cc @dotnet/wasm-contrib @dotnet/jit-contrib @AndyAyersMS |
There was a problem hiding this comment.
Pull request overview
This PR adjusts CoreCLR JIT lowering for Wasm so that when a struct return is lowered to a primitive integer return type, an integer constant return value is retyped to match the lowered native return type. This avoids generating Wasm code with a stack type mismatch (e.g., emitting an i32.const where an i64 return is required).
Changes:
- In
Lowering::LowerRetStruct, underTARGET_WASM, retypeGT_CNS_INTreturn constants togenActualType(nativeReturnType)when the actual types differ. - Add a Wasm-specific comment explaining the typed value stack requirement and the intended effect on emitted constants.
When LowerRetStruct retypes a struct return to a primitive nativeReturnType and the return value is a GT_CNS_INT, the integer branch left the constant as TYP_INT even when the native return type is wider (e.g. TYP_LONG). On register targets this is harmless because integer registers are untyped, but wasm has a typed value stack, so codegen emitted i32.const 0 for an i64-typed return. This surfaced validating the R2R-compiled CoreLib for wasm: Vector64<byte>.get_Zero returns a zero-initialized 8-byte struct, which the wasm ABI lowers to an i64 return. The emitted body ended with i32.const 0 and wasmtime rejected the module with "type mismatch: expected i64, found i32". Retype the integer constant to the native return type under TARGET_WASM so codegen emits an i64.const. Applies to both browser and wasi, which share the JIT. Also submitted standalone as dotnet#129102; will reconcile if that lands first. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
I would have sworn we fixed this already. Let me see... we found it but I can't tell without a bit more work if we ever fixed it. |
Definitely looks like the same issue. It doesn't look like a real fix ever landed, only aworkaround. As you noted, the bad int constant originates in assertion prop (ZeroObj → CNS_INT int 0); I fixed it in LowerRetStruct — your "GT_RETURN inlowering" option — kept minimal and TARGET_WASM-scoped so it covers browser too. Happy to move it to the assertionprop source instead if you'd prefer atarget-independent fix. |
|
It looks intended there at the time? runtime/src/coreclr/jit/assertionprop.cpp Lines 4212 to 4219 in ea282a9 |
| { | ||
| assert(varTypeUsesIntReg(nativeReturnType)); | ||
| #ifdef TARGET_WASM | ||
| // Wasm has a typed value stack, so the constant's type must match the |
There was a problem hiding this comment.
If the sizes don't match, should we do this only for zero (like float cases just above)?
Per review feedback, scope the integer-constant retype to the zero constant, mirroring the float/double case just above. The INT-under-wider struct return only arises from ZeroObj assertion propagation (always zero); a nonzero size-mismatched constant could be widened incorrectly, so gate on IsIntegralConst(0). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the manual ChangeType with BashToZeroConst(nativeReturnType), the purpose-built helper used to create the zero in the first place (assertionprop), and drop the TARGET_WASM guard so the integer-constant case mirrors the unconditional float/double coercion just above. The retype only triggers for the ZeroObj-produced zero (asserted) and is a no-op on register targets, but produces correctly typed IR everywhere. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
I removed the WASM guard and used BashToZeroConst(nativeReturnType); which appears purpose made for this. Happy to add the ifdef back if needed. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| if (genActualType(retVal) != genActualType(nativeReturnType)) | ||
| { | ||
| assert(retVal->IsIntegralConst(0)); | ||
| retVal->BashToZeroConst(nativeReturnType); | ||
| } |
There was a problem hiding this comment.
I think instead of an assertion this should just be part of the if condition
Note
This PR was generated with the assistance of GitHub Copilot.
Problem
When
Lowering::LowerRetStructretypes a struct return to a primitivenativeReturnTypeand the return value is aGT_CNS_INT, the integer branch leaves the constant asTYP_INTeven when the native return type is wider (e.g.TYP_LONG). On register targets this is harmless because integer registers are untyped, but wasm has a typed value stack, so codegen emitsi32.const 0for ani64-typed return.Where it surfaced
Validating the R2R-compiled
System.Private.CoreLibfor wasm.System.Runtime.Intrinsics.Vector64<byte>.get_Zeroreduces (after the unsupported-base-type check inlines away forbyte) toreturn default;— a zero-initialized 8-byte struct, which the wasm ABI lowers to ani64return. The emitted body ended withi32.const 0, and wasmtime rejected the module:This is the first small-struct-constant return in corelib's code section; with it fixed the entire corelib validates.
Fix
Retype the integer constant to the native return type under
TARGET_WASMso codegen emits ani64.const. The change isTARGET_WASM-guarded and does not affect other targets. It applies to both the browser and wasi wasm targets, which share the same JIT.Validation
System.Private.CoreLibwith the fixed JIT; the full module now passeswasmtimevalidation (previously failed atVector64<byte>.get_Zero).i64.const 0instead ofi32.const 0.I held off on adding a dedicated regression test since this class of bug is caught implicitly once the wasm R2R corelib is built and validated/loaded (corelib itself contains the triggering method). Happy to add a targeted test if reviewers prefer an earlier, JIT-level signal.