Repository navigation
fix(relinker): lower scalar RCPSS and RSQRTSS - #11192
RINNECODER wants to merge 4 commits into
Conversation
--to-intel and --to-rosetta replaced only the packed reciprocal register forms, so RCPSS, RSQRTSS, VRCPSS and VRSQRTSS kept the host approximation. Their register forms now use SQRTSS and DIVSS like the packed stubs. The upper lanes come from the destination for the legacy forms and from the first source for the VEX forms, which also clear the upper YMM bits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
Independent local check at head 825fed94f823600bbffba440d8d5e26a7b7b14a7: the relinker-only Release build completes, and the isolated reciprocalOperands, reciprocalExecution and converterReciprocal tests pass, including execution of the generated scalar stubs. The full amd64_only_converter CTest stops earlier in the existing xsaveoptExecution test with SIGILL on this host; /proc/cpuinfo exposes XSAVE but not OSXSAVE, so I cannot count that full-suite run as evidence against this change. Conventions report zero errors and the disclosed debt-length warning (1,068 characters). Hosted Ubuntu relinker and ASan checks pass; Windows and macOS checks are still pending.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
roboterspinne
left a comment
There was a problem hiding this comment.
Reviewed 825fed9 merged onto main 570089a (2026-10-26); the merge is clean. Windows 11, i9-12900H, WinLibs GCC 15.2, relinker-only Release.
Results
- Full
ctest: 308 of 309 pass;patch_advisor_unitfails the same way on plainmain.amd64_only_converterpasses, but on Windows it runs only the decode and convert checks. The execution checks compile only for Linux and macOS x86-64 (Amd64OnlyConverterTests.cpp:1505), so this run executes no stub. - To run the stubs natively, I linked a probe against the built converter objects. It runs each lowered site and the original instruction from the same state: all 16 YMM registers with distinct patterns, RFLAGS, and MXCSR 0x1F80 and 0x9FC0. That gave 27 scalar encodings × 21 inputs × 2 MXCSR values. Outside lane 0 of the destination and MXCSR, the stub matches the native instruction everywhere:
- legacy keeps lanes 1-3 and bits 128-255;
- VEX takes lanes 1-3 from vvvv and zeroes the upper bits;
- xmm8-15 via REX.R/B, VEX.R/B and vvvv ≥ 8;
- identical registers, and vvvv equal to the stub's scratch register xmm0;
- VEX.L=1 and VEX.W=1.
- The scratch register is spilled and restored with its upper bits, and RFLAGS, rsp and rbx are unchanged.
- (V)RCPSS gives the correctly rounded 1/x in every case, and NaN handling matches native.
- The matcher leaves memory forms, VEX pp=1/3 and the 256-bit forms alone.
- Counter-check: with
main'sReciprocalOperands.cpp,ReciprocalLowering.cppandAmd64OnlyInstructionMatcher.cpp(headers kept), the test fails at "VEX2 VRSQRTSS was not decoded". Reverting only the matcher gives your "Scalar reciprocal was not lowered", and reverting the headers too doesn't compile. With your files it passes.
Should fix (text): see the inline comment on the debt entry. The RSQRT forms are not correctly rounded.
Minor: see the inline comment on ReciprocalLowering.cpp. The stub differs from the native instruction in MXCSR behaviour, and the debt entry doesn't record it.
Nits
-
Several wrong variants pass the execution checks, because those use only xmm2 and xmm5 and read only the low 128 bits:
- masking vvvv with 7;
- swapping or dropping the R/B bits in
_vex; - emitting the legacy form so that it clears bits 128-255.
Sites with vvvv ≥ 8 (e.g.
C5 A2 53 D5), a VEX3 destination and source ≥ 8 (e.g.C4 C1 0A 52 D7), and a check of bits 128-255 would catch them. -
Debt wording:
- The scalar forms are VEX.LIG and are lowered with L=1 too, so "VEX.128 register forms" fits only the packed ones.
- "SQRTPS/DIVPS" doesn't match the scalar stub, which emits SQRTSS or MOVSS, then DIVSS and MOVSS.
-
Body:
- The red message holds only when just the matcher is reverted (see above).
- The entry is 1064 characters on
mainand 1068 here, so the PR adds 4, not 3.
Not checked: what Zen 2's RCPSS and RSQRTSS return; the MXCSR that ordinary threads of relinked output run with on Windows and Linux; PPSA04203 or any title run; Linux and macOS x86-64, where the execution checks compile.
For disclosure: my open PRs #11061, #11045, #10971 and #10947 also edit docs/dev/debt/relinker.md; each merges cleanly with this one.
Written with AI assistance (Claude Code). I ran the build, the counter-check and the native probes above on my Windows 11 machine.
| - `--to-rosetta` lowers what Rosetta lacks on top of `--to-intel` ([matcher](../../../core/relinker/codegen/src/x86/Amd64OnlyInstructionMatcher.cpp)). AVX2, FMA, BMI1/2, F16C and MOVBE were observed to execute on an M5 Max without being reported through CPUID; this is not a guarantee for other Rosetta hosts. ANDN and RDRAND fault on an M1 running macOS 15.3.1 and execute on an M1 running macOS 26.6.2, so on the M1 the difference follows the macOS version; on that host BEXTR, BLSI, BLSMSK, BLSR, TZCNT, LZCNT, POPCNT, BZHI, MULX, PDEP, PEXT, RORX, SARX, SHLX, SHRX, MOVBE, FMA, AVX2 and F16C execute as well, its CPUID leaf 1 reports none of MOVBE, FMA, AVX or F16C, and RDSEED, RDPID, CLWB, ADCX and ADOX raise SIGILL. They also execute on an M5 running macOS 26.4, where the rest of BMI1 and BMI2, LZCNT, PREFETCHW, CLFLUSHOPT, AVX, AVX2, FMA, F16C and MOVBE execute as well although CPUID leaf 7 reads zero and leaves 1 and 0x80000001 report none of them, and where RDSEED, CLWB, RDPID, ADCX and ADOX raise an illegal instruction. ANDN is lowered, but RDRAND and other BMI instructions still depend on the host. RDSEED becomes RDRAND and CLWB a NOP. RDPID returns processor 0, and RDTSCP runs unchanged with ECX set to 0 after it, because Rosetta's TSC_AUX is the constant 0xAAAAAAAB (also measured on an M5, macOS 27.0.1). An RDPID or RDTSCP site with no movable instruction after it for the jump is kept; a kept RDTSCP reports 0xAAAAAAAB. ADCX/ADOX fault under Rosetta and are not converted: compilers do not emit them, and code that checks CPUID sees no ADX. The conversions are covered by the converter tests. ANDN lowering is also executed by an x86-64 test build under Rosetta on the M1; the other conversions were originally checked on an M5 Max, outside this repository's CI. | ||
| - `--to-rosetta` output faults on an M2 running macOS 14.8.9 when it reaches AVX, AVX2, FMA, F16C, RDRAND or a BMI1/BMI2 instruction other than ANDN, TZCNT and LZCNT: Rosetta on that host raises SIGILL for them and for RDSEED, RDPID, CLWB, ADCX and ADOX ([matcher](../../../core/relinker/codegen/src/x86/Amd64OnlyInstructionMatcher.cpp)). Only TZCNT, LZCNT, POPCNT, MOVBE, CLFLUSHOPT, PREFETCHW and RDTSCP execute, and RDTSCP sets ECX to 0xAAAAAAAB. CPUID leaf 1 reports POPCNT and none of MOVBE, AVX, FMA, F16C or RDRAND, leaf 7 reads zero, and leaf 0x80000001 reports RDTSCP but neither LZCNT nor PREFETCHW. An M2 Max on macOS 27.2 reports the same CPUID but executes everything except RDSEED, RDPID, CLWB, ADCX and ADOX, so the faults follow the macOS version, not the chip. | ||
| - `--to-intel` replaces the approximate legacy SSE and VEX.128 register forms of RSQRTPS/RCPPS and VRSQRTPS/VRCPPS with [correctly rounded](../../../core/relinker/codegen/src/x86/ReciprocalLowering.cpp) 1/sqrt(x) and 1/x, not with the AMD approximation: its tables are not public. Intel's approximation of 1/sqrt(1.0) is 0x3F7FF000, and two Newton-Raphson steps on it settle on 0.99999994 instead of 1.0; a renormalised identity quaternion then turns into a NaN axis in PPSA21564, which the console does not show. Refined results now reach the exact value; unrefined ones can still differ from the console in the low bits. Each site costs an out-of-line stub (two jumps, a spill below the red zone, SQRTPS/DIVPS). Legacy SSE forms preserve the destination's upper YMM bits; VEX forms clear them. The 256-bit, scalar and memory forms keep the native instruction, as does a site shorter than 5 bytes with no movable instruction after it for the jump (5 sites in PPSA21564, counted before a following RIP-relative instruction could move into the stub; not recounted). | ||
| - `--to-intel` replaces the approximate legacy SSE and VEX.128 register forms of (V)RSQRTPS, (V)RCPPS, (V)RSQRTSS and (V)RCPSS with [correctly rounded](../../../core/relinker/codegen/src/x86/ReciprocalLowering.cpp) 1/sqrt(x) and 1/x, not with the AMD approximation: its tables are not public. Intel's approximation of 1/sqrt(1.0) is 0x3F7FF000, and two Newton-Raphson steps on it settle on 0.99999994 instead of 1.0; a renormalised identity quaternion then turns into a NaN axis in PPSA21564, which the console does not show. Refined results now reach the exact value; unrefined ones can still differ from the console in the low bits. Each site costs an out-of-line stub (two jumps, a spill below the red zone, SQRTPS/DIVPS). Legacy SSE forms preserve the destination's upper YMM bits; VEX forms clear them. The 256-bit and memory forms keep the native instruction, as does a site shorter than 5 bytes with no movable instruction after it for the jump (5 sites in PPSA21564, counted before a following RIP-relative instruction could move into the stub; not recounted). |
There was a problem hiding this comment.
Should fix (text): for (V)RSQRTSS the stub computes SQRTSS and then DIVSS (ReciprocalLowering.cpp:48, :54), which rounds twice, so the result isn't "correctly rounded". Over all 2,139,095,039 positive finite non-zero floats, 556,013,448 results (26%) are one float step off the correctly rounded 1/sqrt(x). Your test input 1.00000012f (0x3F800001) is one of them: the stub gives 0x3F800000, while the correctly rounded value is 0x3F7FFFFF.
The test can't see it, because its reference 1.0f / std::sqrt(value) (Amd64OnlyConverterTests.cpp:1658) rounds twice the same way, while its message says "not correctly rounded". (V)RCPSS is a single DIVSS and is correctly rounded. main's packed RSQRTPS already works this way under the same wording; this PR extends the claim to four more instructions, and the title says "exactly".
Either reword it and the test message, for example "1/sqrt(x) as SQRT then DIV, at most one float step from the correctly rounded value; 1/x correctly rounded", or compute the square-root forms in double precision. (float)(1.0 / sqrt((double)x)) was correctly rounded for every one of those inputs.
| _vex(body, 0, kMovaps, dst, 0, src); | ||
| } | ||
|
|
||
| void _emitScalar(StubBodyBuilder& body, const ReciprocalOperands& operands, const std::uint8_t scratch) { |
There was a problem hiding this comment.
Minor: on main the four scalar forms stay native, and the native instructions set no MXCSR status flags and never raise #XM. This stub does both, as main's packed stub already does. Measured natively, each case in its own process:
- It sets ZE, IE, DE, OE, UE and PE as the inputs dictate.
- With an exception unmasked, the stub traps where the native instruction runs through: ZM with x = 0, PM with x = 3, DM with a denormal, IM with x = -1 (RSQRT forms), UM with x = 0x7F7FFFFF (RCP forms).
- With DAZ and FTZ off, results differ. RCPSS of 0x00400000 gives 0x7F000000 instead of inf, and RCPSS of 0x7F000000 gives 0x00400000 instead of 0.
Under 0x9FC0 (the fiber default and the macOS main thread) the result differences go away and every exception is masked, but the flags are still set. I found no MXCSR setup for ordinary threads on Windows and Linux, so they likely run with the host default 0x1F80. One sentence in this debt entry would record it.
|
@RINNECODER merged main into your branch to resolve the conflict in |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — approved
The previously reviewed reciprocal-lowering implementation is unchanged in the current branch; the latest merge brings in current main and retains the debt update. The register/flag probe and focused implementation review remain applicable, and the exact current head's hosted relinker matrix passes on Linux, macOS (including x86_64), and Windows. The conventions, labels and eligibility checks pass as well.
The full Linux product job is skipped. The Windows product job passes, but its amd64_only_converter target does not execute the scalar stubs; the earlier reviewer probe is the reported native execution comparison. I did not rerun the probe locally. No blocking issue found; approval covers exact head 2750fe2657c3b723cc82b12691586eaf10d29cce.
|
@RINNECODER @oneandonlydean A short note before this is merged: my Should fix on the debt text (inline comment on
Either option from my inline comment closes it: reword the entry and the test message, for example "1/sqrt(x) as SQRT then DIV, at most one float step from the correctly rounded value; 1/x correctly rounded", or compute the square-root forms in double precision, For disclosure: since my review, my open PRs #11487 and #11883 also edit Written with AI assistance (Claude Code). I re-checked the current head's diff and the lines above today; the numbers come from my earlier run on my Windows 11 machine. |
|
The rounding qualification in the existing comment still applies. Please reword the reciprocal-square-root debt entry and scalar test message to describe SQRT then DIV rather than correctly rounded reciprocal square root. For input |
The (V)RSQRTPS and (V)RSQRTSS stubs compute SQRT then DIV, which rounds twice and can land one float step from the correctly rounded 1/sqrt(x). Say so in the debt entry and the test messages, keep "correctly rounded" for 1/x, and note that the stubs set MXCSR status flags and can raise #XM where the native instructions do not. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks @roboterspinne and @oneandonlydean. I reworded the debt entry and the scalar and packed test messages to describe 1/sqrt(x) as SQRT then DIV, at most one float step from the correctly rounded value, and kept "correctly rounded" for 1/x only. I kept the lowering and tests as they are. The entry now also notes that the stubs set MXCSR status flags and can raise #XM where the native instructions do not, which covers the inline note on AI-assisted (Claude Code). |
What
--to-intel(and--to-rosettaon top of it) lowered only the packed reciprocal register forms. The scalar RCPSS, RSQRTSS, VRCPSS and VRSQRTSS were left native, so they kept the host's approximation, while the packed forms right next to them got the exact value.docs/dev/debt/relinker.mdlisted this gap.The register forms of all four now go through the same stub as the packed forms: SQRTSS (RSQRTSS only), then DIVSS into 1.0. The upper lanes are merged the way the instructions define them. The legacy forms keep the destination's lanes 1-3 and its upper YMM bits. The VEX forms take lanes 1-3 from the first source (VEX.vvvv) and clear the upper YMM bits. VEX.L is ignored, as it is for these LIG instructions. Memory forms and the 256-bit packed forms still keep the native instruction, as before. The debt entry is cut to match. The two VEX scalar names are added to the per-name stub summary in
Conversion.cpp, the same as VRSQRTPS and VRCPPS.Title evidence: PPSA04203 (Sid Meier's Civilization VII) has 231 of these in its eboot, all VEX register forms (225 VRSQRTSS, 6 VRCPSS, no memory forms, no legacy forms). With this change,
--macos --to-rosettaon that eboot reports:On
mainthese sites stay native. I have not observed a visible wrong result in that title that this change fixes, so the Unblocks line is none.tools/check_conventions.pyreports one warning,debt-length, on the edited entry: 1068 characters. Onmainthe entry is already 1065 characters, and this change makes it 3 characters longer.Tested
macOS 27 arm64, Apple clang, relinker-only Release build (
-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON):amd64_only_converter(native arm64) runs the decoder and converter checks. It fails onmainwith the new cases (Scalar reciprocal was not lowered) and passes with this change.amd64_only_converterbuilt with-DCMAKE_OSX_ARCHITECTURES=x86_64and run under Rosetta also executes the stubs. It fails onmain(Register form stub was not produced) and passes with this change. The execution checks cover legacy and VEX encodings, distinct and identical registers, a VEX first source equal to the destination and equal to the source, and 1/sqrt(1.0) == 1.0. Each lane is compared bit for bit with1.0f / std::sqrt(x)or1.0f / x. I also broke the stub on purpose twice (MULSS in place of DIVSS, and MOVAPS in place of the final MOVSS), and each break failed the test.tools/check_conventions.py --base upstream/main: 0 errors and 1 warning (see above).relinkertarget builds. Not tested on Linux or Windows, and not tested in a title run.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code