Repository navigation
perf(fpx): keep the hardware result for FMUL/FMLA S when the flags are already set - #917
Conversation
hckrman101
left a comment
There was a problem hiding this comment.
No correctness problem found. I checked a64_fpx_sticky32 against the exact body (EmitFPMulAddValue in suyu/arm64_to_c.h) and Arm's FPMulAdd/FPRound:
RECOMP_FPX_OPENalready requires RN, no DN, no AH/FIZ/NEP, no trap enables and IXC set, so other rounding modes and DN can't reach it, and IXC never needs adding.- The FZ input flush only happens with IDC already set.
- Tininess is detected before rounding: results <= the smallest normal are treated as possibly tiny, and exactly the smallest normal falls back.
- The binary64 cancellation check compares the exact product with the negated addend (no binary64 sum), so it's exact.
- An overflow needs OFC.
- The FNMUL/FMLS/FNMLA negation is outside the call and symmetric under RN.
- The real FMA is only used under
__FMA__with GCC/clang.
Nice work, and a nicely adversarial test. A few points, mostly about what the test can and can't catch, which I'd like answered first:
Test coverage (inline)
- The "rescue decided cases" counter doesn't observe the generated wrappers.
- Only the x86-64-v3 build is tested, so the
A64_FPX_FMA32 == 0path never runs. gen_statenever sets FPCR bits 0-2, and the sweep only uses two flag states.
Design
4. The rescue lives only in the generated outlined wrappers, not behind the runtime's recomp_fpx_* / recomp_fpx_shadow layer. The shadow build emits the inline form (EmitFPNativeSuffix), which FPX_GUARD doesn't rewrite, so the shadow build (which validates the fast paths against the exact body in-game) never exercises this path. Also, the FPX1 contract comment in arm64_to_c.h ("the result is finite and larger in magnitude than the smallest normal, so it is neither NaN, infinite nor tiny...") no longer matches what lifted code keeps. It needs at least a note there pointing to the wrappers, or the rescue could move down into the runtime header so the shadow build covers it.
Smaller things (inline)
5. A64_FPX_FMA32 duplicates RECOMP_FPX_FMA64.
6. Some dead mul32 code is left over.
7. A minor perf note on the fall-through path.
| const uint64_t v = mul ? a64_fpx_mul32_fma32(&got, a, b, z) : a64_fpx_fma32_fma32(&got, a, b, z); | ||
| checks++; | ||
| { Ctx k = got; uint64_t t; k.fpsr = sr; /* (did the sticky rescue decide this one) */ | ||
| rescued[mul] += RECOMP_FPX_OPEN(&k) && !(mul ? recomp_fpx_mul32(a, b, RECOMP_FPX_FZ(&k), &t) |
There was a problem hiding this comment.
This counts by calling a64_fpx_sticky32 directly, so it doesn't show that the generated wrappers ever reach it. Say the (fast, exact) in (("mul32","fma32"),("fma32","fma32")) branch in fpx_header (a64_lift.py ~724-725) stopped matching, or passed the wrong args. The wrappers would then always return recomp_fpe_fma32, which matches the reference, so there'd be no mismatch, and these counters would still be nonzero. So the "can't pass by never running" guarantee doesn't really hold.
Could you count from inside the wrapper path instead? For example, a probe macro (empty by default) in the sticky branch of the generated wrapper, which this driver defines to bump rescued[mul].
| import toolchain # noqa: E402 | ||
|
|
||
| RANDOM_TRIPLES, SEED, SWEEP_STRIDE = 8000000, 20261109, 97 | ||
| FLAGS = ["-march=x86-64-v3", "-ffp-contract=off", "-fno-strict-aliasing", "-O2"] # (nx2/build.py's lifted-code FP flags) |
There was a problem hiding this comment.
This only builds at -march=x86-64-v3, so the A64_FPX_FMA32 == 0 side is never compiled or checked: the SSE-multiply rescue for FMUL and the else return 0; for FMLA. That's what x86-64-v2 builds get from host_march() (used by nx2/build.py), e.g. the Ivy Bridge from issue 363, so it's a real configuration. And on a non-AVX2 machine the whole test skips.
Could you add a second build at -march=x86-64-v2? It can run without the AVX2 check.
| if (rbelow(2)) cr |= 1u << 24; /* FZ */ | ||
| if (rbelow(10) == 0) cr |= 1u << 25; /* DN (gate closed) */ | ||
| if (rbelow(10) == 0) cr |= (uint64_t)rbelow(4) << 22; /* RMode */ | ||
| if (rbelow(40) == 0) cr |= r32() & 0x07ff9f00u; /* anything else */ |
There was a problem hiding this comment.
0x07ff9f00 never sets FPCR bits 0-2 (FIZ/AH/NEP), and the sweep only uses FPSR 0x9f or 0x10. So:
- The AH exclusion the rescue relies on is never exercised. With AH=1, Arm detects tininess after rounding and doesn't set IDC on the input flush. If
RECOMP_FPX_FPCR_MASKlost bits 0-2, this test would still pass. - Partial flag states (e.g. UFC set and IDC clear, with FZ, a subnormal operand and a tiny result) aren't swept.
Could you add bits 0-2 to the "anything else" mask and sweep a few more FPSR combinations?
| exact. Otherwise a result at most the smallest normal may be tiny (UFC): FZ clear, the IEEE gradual-underflow result is | ||
| Arm's; FZ set, below the smallest normal the exact value is tiny and Arm returns a zero of its sign, and exactly the | ||
| smallest normal is ambiguous (falls back). Without a hardware FMA only the multiply is rescued. */ | ||
| #if RECOMP_FPX_HOST == 1 || (RECOMP_FPX_HOST == 2 && defined(__FMA__) && (defined(__GNUC__) || defined(__clang__))) |
There was a problem hiding this comment.
This duplicates RECOMP_FPX_FMA64's condition (suyu/arm64_to_c.h ~6568), which is already in scope here. Could you define A64_FPX_FMA32 from it (or just use it), so the "correctly rounded hardware FMA" test can't drift?
| if fast in ("add32", "sub32", "mul32"): | ||
| if (fast, exact) in (("mul32", "fma32"), ("fma32", "fma32")): | ||
| fz = "if (RECOMP_FPX_OPEN(c) && a64_fpx_sticky32(c, a0, a1, a2, %d, &v)) return v; " % (fast == "mul32") | ||
| elif fast in ("add32", "sub32", "mul32"): |
There was a problem hiding this comment.
Cleanup: FMUL's exact body is always fma (EmitFPOutlined maps mul to fma), so mul32 only ever pairs with fma32, and that pair now takes the branch above. That leaves unreachable code:
"mul32"in thiselifa64_fpx_fz_mul32(~626 and ~631)a64_fpx_fz_result'smulparam and its(mul && m == 0x00800000u)branch (~617)
Could you remove them and update the FPX_FZ_HEADER comment?
| fz = "" | ||
| if fast in ("add32", "sub32", "mul32"): | ||
| if (fast, exact) in (("mul32", "fma32"), ("fma32", "fma32")): | ||
| fz = "if (RECOMP_FPX_OPEN(c) && a64_fpx_sticky32(c, a0, a1, a2, %d, &v)) return v; " % (fast == "mul32") |
There was a problem hiding this comment.
Minor perf: every slow-path call now pays for the hardware op and the checks before falling back. The old FMUL rescue exited cheaply on A64_FPX_FZ_OPEN. NaN operands, and the cases where the needed IDC/OFC/UFC bit is clear, could be rejected before computing. Also, RECOMP_FPX_OPEN(c) is tested twice in the generated wrapper (once for the fast path and again here).
…e already set The fma32-shaped wrappers (a64_fpx_mul32_fma32, a64_fpx_fma32_fma32) fell back to the exact soft-float body whenever the fast path declined: a subnormal operand under FZ, a tiny or infinite result. FPSR's cumulative flags are sticky, so when every flag the Arm op could raise is already set, one hardware FMA (or multiply) result is the Arm result: a subnormal operand under FZ flushed to a zero of its sign with IDC set, a tiny result with UFC set, an infinite result with OFC set, an exact cancellation. a64_fpx_sticky32 keeps those, in place of a64_fpx_fz_mul32 for these two shapes, and falls back otherwise. tests/test_fpx_sticky.py checks both wrappers as the generator emits them against recomp_fpe_fma32, value and FPSR: 116M checks, 0 mismatches. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…FIZ/AH/NEP, drop dead mul32 code Review of NotRexed#917: - The generated wrappers call A64_FPX_STICKY_PROBE() (empty in the real build) when the sticky rescue keeps a result; the test counts there instead of calling a64_fpx_sticky32 itself, so a broken route (fpx_header's branch, the mul argument) fails it (exit 4). - The test builds and runs twice: -march=x86-64-v3 and x86-64-v2 (RECOMP_FPX_FMA64 off: FMUL rescued by a plain multiply, FMLA must decline every case). v2 runs without the AVX2 check. - gen_state sets FPCR bits 0-2 (alone and in the random mask); the sweep adds one state per a: partial flags, IXC clear, FIZ/AH/NEP. A rescue under a closed gate fails the test (the exact body models none of FIZ/AH/NEP, so values alone cannot tell). - A64_FPX_FMA32 is gone: the helper uses RECOMP_FPX_FMA64. - Dead code removed: a64_fpx_fz_mul32, a64_fpx_fz_result's mul param, "mul32" in the fz elif, A64_FPX_FZ_OPEN; FPX_FZ_HEADER's comment updated. - The wrapper tests RECOMP_FPX_OPEN once; a64_fpx_sticky32 rejects a NaN operand, and FMUL with OFC and UFC clear and nothing flushed, before computing (same rescue counts with and without these rejects). - FPX1's contract comment in arm64_to_c.h points to the outlined wrappers' rescues and says the shadow build does not reach them. tests/test_fpx_sticky.py: 2 x (16M random + 150M sweep) checks, 0 mismatches. Each sabotage in its docstring fails it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…'s emit route tests/test_fpx_sticky.py gains a "lifted" case in both builds: four hand-encoded guest words (FMUL, FNMUL, FMADD, FMLA by element, S) go through a64emit.translate and a64_lift.op_text, the lifter's per-instruction pipeline (and hot_text, the hot functions' form), so the C calls the outlined wrapper exactly as lifted code does. Each runs against the same word translated with FPX off (the exact body inline) on 5 inputs that must be rescued and 7 that must fall back (IXC only, UFC/OFC/IDC clear, NaN, FZ|AH, a normal result): whole Ctx and the A64_FPX_STICKY_PROBE count must match. 96 runs a build, 0 mismatches; 40 rescues at x86-64-v3, 20 (FMUL/FNMUL only) at x86-64-v2. a64_lift.py: Lifter.write's per-instruction pipeline and the hot-function rewrite become op_text() and hot_text(), which the test calls (no change to what they emit). Sabotage: the emitter no longer outlining FMUL S (EmitFPMulAddValue's `FpxOutline()` made `FpxOutline() && !mul`) fails the lifted case in both builds (20 FMUL/FNMUL runs without a rescue) while the random and sweep runs, which call the wrappers by name, pass. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e5f07b2 to
0a2b38e
Compare
|
Thanks. All seven points are addressed in 1ff4abe and 0a2b38e:
Results: 2 × (16M random + 150M sweep + 96 lifted) checks, 0 mismatches. |
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, went through 1ff4abe and 0a2b38e. All seven points are covered:
- The probe sits in the wrappers' sticky branch, and the lifted case runs real guest words through
op_text/hot_text, so the count now comes from the code the build actually emits. - The v2 build plus the
#erroragainstRECOMP_FPX_FMA64covers the no-FMA side, with FMLA required to stay at 0 there. - With the test's own copy of the gate mask, a rescue under FIZ/AH/NEP now fails the test even when the value matches. That's the part a value comparison couldn't catch.
I also checked the new early rejects. NaN, IDC clear with a flushed operand, and FMUL with OFC/UFC both clear only drop cases the old code already returned 0 for. Taking RECOMP_FPX_OPEN out of the helper is fine, since both wrappers now call it inside the gate, and the add/sub condition matches the old A64_FPX_FZ_OPEN.
LGTM.
Brings in motion controls (NotRexed#1114), 5.1 surround (NotRexed#1182), the pipeline cache over 2 GB (NotRexed#1226), the queued pipeline build run at once (NotRexed#1129), the floating-point fixes (NotRexed#917), the builder's cleanup stop (NotRexed#1218) and the render target pool's 512 MB minimum. Conflicts, resolved for both sides: - gpu/vk/vk_api.h: main's motion rows took option ids 18 to 22, which DLSS's camera jitter used (18 to 20). DLSS's now start at 24 (HOS_OPT_DLSS_JITTER_W, _H, _PHASES); every use is by name (host/rx_backend.c, vk_replay.cpp). - gpu/vk/vk_replay.cpp: the option table answers both the motion rows and DLSS's jitter. - gpu/vk/vk_menu.cpp: the motion rows in this branch's Row shape and help style: - Motion controls gets a short help and a hint for each value (hn_gyro). Which pads have a gyroscope moved to On's hint. - The sensitivity and invert rows pass no hint and no restart before preset_exempt, as main had them. - tests/test_rumble_menu.py: main's reading of every HOS_OPT_* enum (it covers DLSS's jitter ids too). Also: - tests/test_output_resolution.py: the render target pool is now 512 MB at least (main's nn_applet.c), so the test expects 512, not 305. Main's own copy still expects 305 and fails.
What this fixes
Single-precision FMUL/FNMUL/FMLA results that the FPX1 fast path can't prove fell back to the soft-float exact body every time. These are tiny or infinite results, or a subnormal input under FZ. Code that runs with those cumulative FPSR flags already set paid that cost on each such op.
Why merge
The rescue keeps the hardware result only when it can raise no new FPSR bit. Everything else still goes to the exact body. The review found no correctness problem.
Change
a64_fpx_sticky32runs in the outlined wrappersa64_fpx_mul32_fma32anda64_fpx_fma32_fma32, inside the gate.RECOMP_FPX_FMA64; without a hardware FMA only FMUL is rescued.fz_mul32code is removed.Tested
tests/test_fpx_sticky.pybuilds at x86-64-v3 and x86-64-v2. Each build runs:There were 0 mismatches against the exact body. Rescues are counted inside the wrappers, and a rescue under a closed gate fails the test. Sabotages of the emitter route, the wrapper route, the FIZ/AH/NEP mask and the UFC/IDC checks each fail it.
Not covered
🤖 Generated with Claude Code