JIT: Improve parameter register field extraction and reconstruction#131174
JIT: Improve parameter register field extraction and reconstruction#131174lewing wants to merge 13 commits into
Conversation
The parameter-register-to-local mapping in lowering maps a scalar field of an unenregisterable parameter to the local created for the containing register segment, retyping the local access to the field's scalar type. On x64/arm64 a scalar overlaps the low bytes of the vector register, so reading the register local at scalar width is a free, correct reinterpret. On wasm this is invalid: a v128 and a scalar (f64/f32/...) are distinct value-stack types with no implicit reinterpret, so 'local.get <v128>' typed as a scalar produces invalid wasm (wasm-tools: 'expected f64, found v128'). This surfaces when crossgen2 compiles, e.g., Vector128<T>.GetHashCode (whose GetElementUnsafe reads a scalar lane of the vector) inlined into GenericEqualityComparer<Vector128<double>>.GetHashCode. Guard the mapping on wasm: skip mapping a scalar field to a SIMD register segment so the access falls back to the (memory-homed) unenregisterable parameter and a normal scalar load is emitted. This is the floating-point ToScalar / SIMD->scalar var_type case noted as deferred in dotnet#130444.
|
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. |
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR adjusts JIT lowering for the wasm target to avoid inducing an invalid scalar read from a SIMD (v128) register-local when the accessed parameter field is a non-SIMD scalar type. The change is localized to the parameter-register-local mapping logic and is guarded to affect only wasm.
Changes:
- Add a
TARGET_WASM-guarded check inLowering::FindInducedParameterRegisterLocalsto skip mapping when the ABI segment is SIMD but the field access is scalar. - Preserve existing behavior for non-wasm targets and for SIMD-typed field accesses.
|
Is this on top of #130866? Else how do we have a V128 parameter? I agree with Tanner, would be nice to use |
|
Yes — this is effectively on top of #130866 (it's in a wasm R2R prototype branch that has #130866 merged, which materializes `Vector128` as a wasm `v128` parameter — hence the v128 register parameter here). Agreed on `f64x2.extract_lane 0` (and the general extract/insert for the sibling paths) being the better, spill-free lowering. Let me look at doing it that way in this PR rather than the memory fallback. Note This comment was authored with the assistance of GitHub Copilot. |
Per review feedback (thanks @tannergooding, @AndyAyersMS): rather than skipping the parameter-register-local mapping for a scalar field of a SIMD register (which forces a spill/reload through the memory-homed parameter), extract the lane directly from the SIMD register local via gtNewSimdGetElementNode. This lowers to an explicit lane extract (e.g. f64x2.extract_lane 0) with no spill. The GetElement node is lowered by the subsequent per-block lowering pass. > [!NOTE] > This change was authored with the assistance of GitHub Copilot.
…tnet#131174) Extract the scalar lane directly from the SIMD register local instead of a memory spill/reload.
…tnet#131174) Extract the scalar lane directly from the SIMD register local instead of a memory spill/reload.
Use direct SIMD lane extraction and scalar bit manipulation for fields carried in floating-point parameter registers. Keep physical promotion eligibility synchronized and retain the ARM32 fallback. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 22214765-2cd2-4951-b778-8793730f2bc1
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 22214765-2cd2-4951-b778-8793730f2bc1
lewing
changed the title
|
Latest round before narrowing to fp |
Drop the induced-access suppression that only helped contrived HFA forwarding cases and restore direct promotion costing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 22214765-2cd2-4951-b778-8793730f2bc1
Use a SIMD lane insertion for two float fields carried in one floating-point ABI register, avoiding spill/reload sequences for arguments and returns. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 22214765-2cd2-4951-b778-8793730f2bc1
lewing
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/tests/JIT/opt/Unsafe/Unsafe.cs:80
- The construction of
doubleValueassumes little-endian layout (low 32 bits at byte offset 0, high 32 bits at byte offset 4). On big-endian targets this swaps the two floats, soUnsafeAsSecondFloat_Double(doubleValue)will not return 2.5f and the test becomes endian-dependent. BuilddoubleValuewith aBitConverter.IsLittleEndianswap so byte offset 4 always contains the 2.5f bits.
double doubleValue = BitConverter.Int64BitsToDouble(
((long)BitConverter.SingleToInt32Bits(2.5f) << 32) | (uint)BitConverter.SingleToInt32Bits(1.25f));
if (UnsafeAsSecondFloat_Double(doubleValue) != 2.5f)
return 0;
float expectedMisaligned = BitConverter.Int32BitsToSingle((int)(BitConverter.DoubleToInt64Bits(doubleValue) >> 16));
if (UnsafeAsMisalignedFloat_Double(doubleValue) != expectedMisaligned)
## Summary Fixes a WASM codegen bug where extracting a scalar field from a SIMD-register parameter leaks a `v128` where a scalar (`f64`/`f32`/...) is expected, producing invalid WASM: ``` type mismatch: expected f64, found v128 ``` `Lowering::FindInducedParameterRegisterLocals` maps a scalar field of an unenregisterable parameter to the local created for the containing register segment. Native targets can read the overlapping low scalar value by retyping that local access, but WASM has distinct `v128` and scalar value-stack types and cannot reinterpret them implicitly. This is the floating-point `ToScalar` / SIMD-to-scalar `var_type` transition at ABI boundaries noted as deferred in #130444. ## Fix When the mapped incoming register local is SIMD and the field is scalar, use `gtNewSimdGetElementNode` under `TARGET_WASM`. For lane zero this becomes `ToScalar`, which WASM lowers to the appropriate `extract_lane` operation rather than retyping a `v128` local or spilling through memory. The byte offset is required to be scalar-lane aligned before deriving the lane index. Every non-WASM target retains the existing path unchanged. ## Validation - `./build.sh clr.jit -rc Debug` - `PATH="/opt/homebrew/bin:$PATH" ./build.sh -os browser -c Debug -subset clr+libs` - Confirmed `lower.cpp` compiled into the WASM JIT. - `python3 src/coreclr/scripts/jitformat.py -r . -o osx -a arm64` There is currently no WASM R2R validation leg in CI suitable for a focused regression test. The existing generalized extraction tests from #131174 were intentionally not included in this narrowly scoped WASM correctness fix. > [!NOTE] > This PR was authored with the assistance of GitHub Copilot. Copilot-Session: e48939ab-dc73-4630-a8ca-72c3b926133a
Represent two scalar floats as an xarch unpack-low operation, producing smaller and faster code than lane insertion while preserving the exact ABI layout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 22214765-2cd2-4951-b778-8793730f2bc1
Resolve the lower.cpp conflict by retaining the generalized parameter register extraction path, which subsumes the landed WASM-specific fix, while preserving the lane-alignment assertion from main. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 22214765-2cd2-4951-b778-8793730f2bc1
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/tests/JIT/opt/Unsafe/Unsafe.cs:89
- The new bit-pattern construction/expectations for
doubleValueandexpectedMisalignedassume little-endian layout (via<< 32and>> 16). This will miscompute expected values on big-endian targets, causing false failures. It also makes the misaligned expectation dependent on integer shifting rather than the actual in-memory byte layout being tested.
double doubleValue = BitConverter.Int64BitsToDouble(
((long)BitConverter.SingleToInt32Bits(2.5f) << 32) | (uint)BitConverter.SingleToInt32Bits(1.25f));
if (UnsafeAsSecondFloat_Double(doubleValue) != 2.5f)
return 0;
float expectedMisaligned = BitConverter.Int32BitsToSingle((int)(BitConverter.DoubleToInt64Bits(doubleValue) >> 16));
if (UnsafeAsMisalignedFloat_Double(doubleValue) != expectedMisaligned)
return 0;
Vector128<float> floatVector = Vector128.Create(1.0f, 2.0f, 3.0f, 4.0f);
if (UnsafeAsInt_Vector128(floatVector) != BitConverter.SingleToInt32Bits(3.0f))
return 0;
Vector128<double> doubleVector = Vector128.Create(5.0, 6.0);
if (UnsafeAsSecondDouble_Vector128(doubleVector) != 6.0)
return 0;
Use the lowering helper that removes scalar operands when CreateScalarUnsafe folds to a vector constant, preventing unmarked unused LIR values. Cover constant float-pair arguments in the directed StructABI test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 22214765-2cd2-4951-b778-8793730f2bc1
|
Build 1523081 measures the final candidate after merging the WASM extraction fix from
Representative production improvements in the final run include:
The final - sub rsp, 24
- vmovsd qword ptr [rsp+0x10], xmm0
- vmovsd qword ptr [rsp+0x08], xmm1
+ vmovaps xmm2, xmm0
+ vmovshdup xmm0, xmm0
+ vmovaps xmm3, xmm1
+ vmovshdup xmm1, xmm1
+ vaddss xmm2, xmm2, xmm3
vaddss xmm0, xmm0, xmm1
- vmovss dword ptr [rsp], xmm0
- vmovss dword ptr [rsp+0x04], xmm1
- vmovsd xmm0, qword ptr [rsp]
+ vunpcklps xmm0, xmm2, xmm0Code size falls from 53 to 29 bytes, instruction count from 11 to 8, and PerfScore from 17.50 to 10.50. The complete throughput result is small but exposes two distinct costs:
Cross-architecture asm totals are favorable overall, with one new ARM32 caveat:
The ARM64 improvements mostly remove spills while extracting incoming SIMD/register fields. A small number of Linux ARM's +396 bytes are concentrated in a few hard-float
The ARM32 guard still rejects extraction forms that would require late Representative diffsAll examples in this section are from final-candidate build 1523081. Production tuple return:
|
| Shape | Baseline | insertps prototype |
unpcklps alternative |
|---|---|---|---|
| Return | 22 bytes, PerfScore 6.25 | 7 bytes, PerfScore 2.00 | 4 bytes, PerfScore 2.00 |
| Construct + pass | 35 bytes, PerfScore 9.75 | 25 bytes, PerfScore 6.75 | 22 bytes, PerfScore 6.75 |
The functional and disassembly tests pass for arguments and returns. ARM64 is behaviorally unchanged because its HFA ABI uses two separate scalar FP register segments.
BenchmarkDotNet 0.16 preview, local x64 under Rosetta:
- Short run (
insertps): return ratio 0.58; pass ratio 0.84. - Short run (
unpcklps): return ratio 0.78; pass ratio 0.75. - A medium run was bimodal under Rosetta and is not reliable enough to rank return performance.
Native Linux x64 Codespace benchmark (AMD EPYC 7763, x86-64-v3, MediumRun):
| Method | Baseline | insertps |
unpcklps |
|---|---|---|---|
| Return pair | 8.286 ns | 4.199 ns (0.51x) | 2.737 ns (0.33x) |
| Construct + pass | 9.402 ns | 5.362 ns (0.57x) | 4.384 ns (0.47x) |
The return distributions had some multimodality, but their medians preserve the same ordering:
7.559 ns baseline, 4.128 ns insertps, and 2.742 ns unpcklps. The argument baseline was stable
(9.402 ns, 0.035 ns standard deviation), and both direct forms remained substantially faster.
The native reports are archived under native-x64-benchmark/.
The benchmark result is strong enough to distinguish the designs:
- Both direct register-packing forms substantially outperform the stack baseline.
unpcklpsis faster thaninsertpsfor both measured shapes and has the smaller encoding.- This supports the backend capability itself while favoring a different IR/instruction selection than the first prototype.
Reconstruction-site prevalence
Mining the stage-2 Linux x64 raw assembly corpus found 72 changed methods whose diff still contained a
Spilled local for field list:
- 36 CoreCLR stress methods, dominated by HFA forwarding.
- 14 library PMI methods.
- 12 library crossgen2 methods.
- 9 non-tiered library-test methods.
- 1 benchmark method.
The production set includes ComplexFloat, PointF, SizeF, and RectangleF methods. Most already net-improve
from extraction, but the remaining spill local shows that direct reconstruction is a recurring backend shape,
not only an HFA stress artifact. The mined records are in s2-field-list-spills.json.
The build-1522492 insertion footprint is in build-1522492-insertps-methods.json.
ISA and representation
- The runtime's x64 and default R2R baseline is x86-64-v2, which includes SSE4.1.
insertpsis therefore legal baseline code. DOTNET_EnableSSE41=0andDOTNET_EnableHWIntrinsic=0still emittedinsertps; these switches control exposed managed intrinsic support, not removal of x86-64-v2 baseline instructions from internal JIT codegen.unpcklpsexpresses the exact packing operation more directly, uses an older baseline instruction, and saves three code bytes per packing site.- Native x64 benchmarking confirms the xarch-only
unpcklpsrepresentation is stronger thaninsertps.
Remaining architectural limitation
In HFA forwarding stress cases, old promotion extracts fields from an incoming carrier and the backend then inserts them back. Direct insertion removes the reconstruction spill, but it cannot remove the redundant extract/insert pair. Eliminating that pair requires ABI-based promotion or forward substitution, as Jakob described.
Full breakdown at https://gist.github.com/lewing/59292126500746b46abd297f609cfad4
|
Marking ready for review but there is no urgency on my side, the wasm blocker has been resolved separately. |
|
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. |
Summary
Improve both directions of struct field handling for incoming and outgoing ABI registers:
floatfieldsThe urgent WASM correctness fix has been split into #131237. This PR remains the broader JIT exploration for native code quality and the generalized implementation.
Changes
gtNewSimdGetElementNodefor aligned scalar fields contained in SIMD parameter registers.double-to-second-floatcase emitsmovshdup; ARM64 emitsdup.TYP_LONGnodes after long decomposition.Promotion::MapsToParameterRegisterwith lowering eligibility.floatfields in one 8-byte floating-point ABI register withNI_X86Base_UnpackLow, producingunpcklpsinstead of stack stores and reloads for arguments and returns.Code generation
For a constructed
{ float X; float Y; }value:Both paths now use one
unpcklpsand avoid the spill/reload sequence.Native Linux x64 BenchmarkDotNet results (AMD EPYC 7763):
insertpsprototypeunpcklpsThe HFA forwarding stress methods demonstrate the remaining architectural issue Jakob described: old promotion can extract fields and the backend then inserts them back-to-back. The insertion support removes the reconstruction spill, but eliminating the no-op extract/insert pair requires ABI-based promotion or forward substitution and is intentionally not papered over with a promotion heuristic in this PR.
SuperPMI evidence
The archived
insertpsrun (build 1522492) directly established the backend capability's scope:insertpsimproved, netting -4,643 bytes with no remaining field-list spill.insertps-containing regressions were contrived HFA/ABI stress contexts with redundant extract/insert pairs.The full archived analysis and representative diffs are available at https://gist.github.com/lewing/59292126500746b46abd297f609cfad4. Fresh CI for the
unpcklpsrefinement is pending.Validation
JIT/opt/Unsafe/Unsafeextraction regression testsJIT/Directed/StructABI/FieldListFloatInsertionfunctional and x64 disassembly checks for arguments and returnsDOTNET_EnableSSE41=0,DOTNET_EnableHWIntrinsic=0)unpcklpsloweringNote
This PR was authored with the assistance of GitHub Copilot.