JIT: Improve parameter register field extraction and reconstruction - #131174
JIT: Improve parameter register field extraction and reconstruction#131174lewing wants to merge 17 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
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. |
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.
Suppressed comments (2)
src/tests/JIT/opt/Unsafe/Unsafe.cs:75
- This test constructs a
doublecontaining twofloatbit patterns and then reads the “second float” viaUnsafe.As<double, float>+Unsafe.Add. That interpretation depends on the platform endianness, so it can produce different results (and false failures) on non-little-endian targets. Consider skipping these checks whenBitConverter.IsLittleEndianis false (or computing the expected value based on endianness).
double doubleValue = BitConverter.Int64BitsToDouble(
((long)BitConverter.SingleToInt32Bits(2.5f) << 32) | (uint)BitConverter.SingleToInt32Bits(1.25f));
src/tests/JIT/Directed/StructABI/FieldListFloatInsertion.csproj:11
- The project file enables disasm checking but doesn’t follow the established pattern used by other
<HasDisasmCheck>true</HasDisasmCheck>tests (e.g.,src/tests/JIT/opt/Unsafe/Unsafe.csproj) to force process isolation and disable tiered compilation/minopts. Without these, the disasm checks can become flaky or validate Tier0 codegen instead of the intended optimized codegen.
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<DebugType>None</DebugType>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs">
<HasDisasmCheck>true</HasDisasmCheck>
</Compile>
</ItemGroup>
</Project>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
src/tests/JIT/opt/Unsafe/Unsafe.cs:80
- This test constructs and inspects bit patterns via Int64BitsToDouble/DoubleToInt64Bits shifting, which assumes little-endian layout for {float,float} within a double and for the byte-offset-2 misaligned read. On big-endian targets these expectations won’t match the actual memory layout. Consider building the double from a byte[]/Span composed from BitConverter.GetBytes(float) and computing expectedMisaligned via BitConverter.ToSingle(bytes, 2) to make the test endian-agnostic.
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)
FieldListFloatInsertion did not set RequiresProcessIsolation, so it was compiled into the merged test runner and executed in-process. The generated run script that exports DOTNET_JitDisasm and invokes SuperFileCheck was never used, leaving the unpcklps assertions dead. Add RequiresProcessIsolation along with DOTNET_TieredCompilation=0 and DOTNET_JITMinOpts=0, matching every other disasm-check project that uses positive checks. Also require FEATURE_HW_INTRINSICS alongside TARGET_XARCH for the float-pair reconstruction blocks, since InsertNewSimdCreateScalarUnsafeNode and gtNewSimdHWIntrinsicNode are only declared under that feature. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
JIT/Directed uses hand-maintained MergedWrapperProjectReference lists rather than the glob that JIT/opt uses, and the new project was never added to one. No runner referenced it, so the test never ran in CI in any form - it appears in neither the passed nor the skipped results for the Pri0 legs. Building it directly with -Test or -Dir hid this, since those bypass runner discovery. Add it to Directed_ro.csproj alongside the other StructABI projects; DebugType None with Optimize true matches that runner. The generated runner now registers it as an out-of-process test, the same shape as EmptyStructs.cmd. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Summary
Improve both directions of struct field handling for incoming and outgoing ABI registers:
floatfieldsThe urgent WASM correctness fix was 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):
insertpsprototypeunpcklpsSuperPMI results
Latest CI run (build 1565991). Collections reporting code size differences:
libraries.crossgen2.linux.x64.checkedlibraries.crossgen2.osx.arm64.checkedsmoke_tests.nativeaot.linux.arm64.checkedsmoke_tests.nativeaot.windows.arm64.checkedlibraries_tests_no_tiered_compilation.run.linux.arm64.Releasecoreclr_tests.run.linux.arm.checkedlibraries_tests_no_tiered_compilation.run.linux.arm.ReleaseThe archived earlier analysis, including the
insertps-versus-unpcklpscomparison and representative diffs, is at https://gist.github.com/lewing/59292126500746b46abd297f609cfad4.Known ARM32 regression
ARM32 is net +314 bytes and is the one area I would like reviewer input on before merge.
RayTracer:GetNaturalColor(SceneObject,Vector,Vector,Vector,Scene)SIMDTests.Vector3InteropTests.PInvokeTest:nativeCall_PInvoke_Vector3ArgHFATest.TestMan:Average19_HFA02Runtime_128373:ProblematicBodyCamera:CreateThis comes from synchronizing
Promotion::MapsToParameterRegisterwith lowering. On ARM32, which has noFEATURE_SIMD, eligibility changes from float register with integer access at the segment offset to float register with floating-point access at the segment offset. Promotion therefore becomes more willing to split floating-point parameter registers intofloatreplacements.The float-pair reconstruction added here is
TARGET_XARCHonly, so ARM32 has no way to rebuild the aggregate and must spill and reload at P/Invoke argument sites. That is the same extract-then-reinsert shape @jakobbotsch described, on a target that lacks the backend support which makes it profitable on x64.The options I see are to accept this, or to keep
MapsToParameterRegisterconservative on targets where the backend cannot reconstruct. The latter re-narrows the synchronization this PR is trying to achieve, so I would rather agree on the direction than pick one unilaterally.Test infrastructure
JIT/Directed/StructABI/FieldListFloatInsertionwas not actually running in CI, so itsunpcklpsassertions were never evaluated. Two separate causes:JIT/Directeduses hand-maintainedMergedWrapperProjectReferencelists rather than the globJIT/optuses, and the new project was never added to one. No runner referenced it, so it appeared in neither the passed nor the skipped results of the Pri0 legs.RequiresProcessIsolation, so even once referenced it would have run in-process, and the generated run script that exportsDOTNET_JitDisasmand invokes SuperFileCheck would never have been used.Both are fixed, along with
DOTNET_TieredCompilation=0andDOTNET_JITMinOpts=0to match the other disasm-check projects that use positive checks.Validation
JIT/opt/Unsafe/Unsafeextraction regression tests, confirmed emittingmovshdupJIT/Directed/StructABI/FieldListFloatInsertionon osx-x64: SuperFileCheck resolves all three methods and the disasm output contains threeunpcklpsinstructionsDOTNET_EnableSSE41=0,DOTNET_EnableHWIntrinsic=0)unpcklpsloweringNote
This PR was authored with the assistance of GitHub Copilot.