Repository navigation
perf(shader): drop f16 NaN payload rebuilds that the result never uses - #11368
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head aa3682a15825f3cfef0034ed5652d222295baa5b. I found a blocking test-integration gap:
The new tests/execution/Float16UnusedNanPayloads.cpp is not referenced anywhere in the PR's core/libs/prx/libSceAgcDriver/CMakeLists.txt; neither a target nor a CTest entry is added. Existing AGC tests are explicitly registered there. As submitted, CI does not compile or run the test described in this PR, so the 20,971,520-result comparison is not an enforced regression test. Please register the new executable and test in CMake with the appropriate GPU-test environment/skip handling, then verify the test is listed and runs through CTest.
The test currently performs about 45,000 dispatch-and-wait cycles (packed and unary cases each run both compiled variants). Repository guidance requires tests to finish within 30 seconds on CI runners, including lavapipe; please report its runtime on that path and batch/reduce work if it exceeds the limit.
The branch is based on cbee224804625f2416993f94b2bc1f8c58b1d4bb, 44 main commits behind current 172dcf29014b90340f6a7ae2fda284e4f5518e41. The current-main merge preview is clean and no touched path changed, but the repository asks branches to stay current. After addressing the test gap, sync to current main and rerun conventions, overlap, and the relevant focused checks. I could not run a Vulkan test locally because this environment has no /dev/dri device.
|
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.
|
v_pk_fma_f16 and the other packed f16 ops computed in f32 replace every NaN lane with the quieted NaN source or 0xfe00, and f16 unary ops replace the result of a NaN input the same way. Their f16 -> f32 input conversions and their f32 -> f16 packing no longer rebuild RDNA's NaN payload: the widening is a plain UnpackHalf2x16 and the packing only keeps the result a NaN. The conversions carry FloatConvertFlags so the emitter knows. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PHJtpJ4hw3KWJhobipK8ia
…h per pass Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PHJtpJ4hw3KWJhobipK8ia
aa3682a to
103cf58
Compare
|
@MarcZX10 thanks. On registration: |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I re-reviewed exact head 103cf586a9fb6cca773d1b77a696b148006f853c and current main 4e43321bd6ceccdc1ae28568bbbc35d9a6446464. I was wrong about test registration in my previous review: tests/execution/*.cpp are picked up by the CMake glob, then turned into executables and CTest entries. I withdraw that finding. The updated test also rebatches the work and the branch is now based on current main.
One test-coverage issue remains in Float16UnusedNanPayloads.cpp: the std::ranges::all_of(..., word == 0xdeadbeefu) check only fails when all eight output words for an invocation remain untouched. If one of the two four-word stores is missing, the other store makes all_of false; the untouched words remain the sentinel in both fast and software, compare equal, and that half of the operation set is silently accepted. Please verify every output slot was written in both buffers (or add explicit per-result write markers) so an omitted packed or unary result cannot pass as a matching sentinel.
Validation on this host: conventions pass, git diff --check passes, current main is an ancestor of this head, and the merge preview with open #11351 is clean. I downloaded the two release traces and summaries and recomputed both trace_digest.py reports; they match the attached reports after normalizing CRLF. I could not run the Vulkan test here because /dev/dri is absent. I read the author's reported NVIDIA/Intel test runs and the current hosted policy checks; product/GPU build jobs are skipped by path policy.
An output word that neither program stores kept the same sentinel in both buffers and compared equal, so a missing store could pass. The device and software runs now start from different words. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PHJtpJ4hw3KWJhobipK8ia
|
@MarcZX10 right, a missing four-word store would have passed. |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Approved after re-reviewing exact head 2a9175b6a6a2cea63eb7dce6aa62a4d77ea9bee1 against current main 4e43321bd6ceccdc1ae28568bbbc35d9a6446464.
The new per-program output markers close the remaining test gap: a word left unwritten by either implementation now differs from the other buffer, and the author's mutation check that omits the second four-word store fails on both RTX 4090 and UHD 770. The author reports the restored exact-head test passing on both GPUs; the previously reported focused validation-layer runs and no-new-VUID results cover the unchanged production implementation. My current-head conventions and git diff --check pass. I did not independently run the GPU test because this host has no /dev/dri device. I also re-ran tools/trace_digest.py for both attached performance traces earlier; both digests matched the published reports after newline normalization.
AI-assisted review.
|
Thanks @MarcZX10 for the reviews and for rerunning the digests. |
imellag
left a comment
imellag
left a comment
There was a problem hiding this comment.
macOS / MoltenVK data point. Apple M4 Pro, macOS 27.2, MoltenVK 1.4.2, x86_64 Release under Rosetta. main doesn't run titles on macOS yet, so the base is a support branch, not main: main 750cf6cf48 + only the macOS boot PRs #7631 #10552 #11249 #11447 #11554 #10669 #7804, + #12529 (the revert of #10337, which roughly doubles the GPU time on this GPU and would hide smaller changes). Change: that base + eight open shader performance PRs merged together (#12287 #11952 #12299 #12280 #11368 #11265 #8716 #8958, all clean merges). I did not measure each PR alone.
Two alternated runs per side, warm shader cache, APS5_TRACE_SUMMARY_EVERY=10:
| base | with the eight PRs | |
|---|---|---|
| God of War Sons of Sparta (PPSA28997), fps from 30 s | 51.5 / 51.8 | 51.6 / 51.8 |
| GoW GPU ms/present, gameplay before the fight / in the fight | 13.6 / 19.6 | 13.5 / 19.5 |
| Planet of Lana (PPSA19450), fps from 85 s | 32.2 / 32.7 | 32.5 / 32.6 |
Together they don't raise the GPU time on MoltenVK. I'm checking open shader performance PRs on this GPU because #10337 was measured on RADV and doubled it here.
Focused tests below ran with ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation on the same two trees; the layer was found and both logs have the same VUID- lines (12 VUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605, 20 VUID-vkCreateDescriptorSetLayout-support-09582, from tests that fail the same way on the base).
This PR on MoltenVK: the change is inert here. MoltenVK has no native f16 conversions with RTE rounding and kept f16 denormals, so the new agc_driver_float16_unused_nan_payloads and agc_driver_packed_float16_native are skipped (the device has no native f16 conversions with RTE rounding and kept f16 denormals), as are agc_driver_float16_dot and agc_driver_fp16_overflow. The software paths it leaves alone pass on the base and with the change: agc_driver_nan_quieting, agc_driver_packed_alu, agc_driver_f16_fma_denormal_modes, agc_driver_f16_fma_invalid_nan, agc_driver_float16_fma_rounding, agc_driver_fma_mix_f16_denormal_modes. No GPU regression in the group. Nothing on macOS for or against the change itself.
AI-assisted: yes (Claude Code ran the builds, runs and tests and drafted this review)
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 483 tests)Skipped (45)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
@tibzejoker here is a run on AMD's Windows driver, which Tested lists as not covered. On this card the change is only reached with #11483 merged too, and there it passes. Setup. RX 9070 XT (RDNA 4,
The test's message is The same
The failure is the check this PR is meant to satisfy, so it works as the red side.
AI-assisted: yes (Claude Code) |
|
Thanks @savvaniss, that fills the gap in Tested: AMD's Windows driver on RDNA 4.
I'll add your table to the body. I'll also cite the RDNA 4 line on #11483, since it shows AI-assisted: yes (Claude Code) |
What
Step 4 of #11183. Packed f16 ops computed in f32 and f16 unary ops no longer rebuild RDNA's NaN payload in conversions whose NaN the result throws away.
v_pk_fma_f16,v_pk_fmac_f16(andv_pk_add_f16/v_pk_mul_f16when they are not native, e.g. withclampor flushed denormals) widen each f16 lane to f32, compute, and pack the two results back withPackHalf2x16. On a device with Float16,RoundingModeRTE16 andDenormPreserve16, each widening rebuilds the NaN payload afterUnpackHalf2x16(6 instructions) and each packed lane rebuilds it afterOpFConvert(11 instructions). ThenpackedFloat16applies RDNA's NaN rule to every lane, from the raw f16 source bits:inf * 0);0xfe00;clampandDX10_CLAMP, the rule is skipped because the clamp already turns a NaN into 0.So the payload of every one of those conversions is overwritten:
inf * 0, the product is NaN whatever the addend is). If no source is NaN, the conversion is exact and has no NaN to rebuild. Only NaN-ness can still matter, through theDX10_CLAMPpath, andUnpackHalf2x16keeps it: today's code already relies on it, since the payload rebuild is selected byOpIsNanon that same result, andOnlyCompareduses the plain unpack already;0xfe00when no source is NaN). The packed lane is nowisNaN(f32) ? 0x7e00 : OpFConvert, tested on the f32 bits, so it does not depend on whatOpFConvertreturns for a NaN.f16 unary ops (
v_rcp_f16,v_sqrt_f16,v_rsq_f16,v_log_f16,v_exp_f16,v_fract_f16,v_sin_f16,v_cos_f16) replace the result of a NaN input with the quieted input bits, so their input widening does not need the payload either. Their f32 -> f16 rounding keeps today's code: the result of a non-NaN input is not always overwritten.The translator marks those conversions with
FloatConvertFlags::nanPayloadUnused(IR value flags, likeMemoryFlags);EmitConvertF32F16then emits the plain unpack andPackHalf2x16the NaN-ness-only lane. The flags are part of the redundant value key, so a marked conversion is never merged with an unmarked one. No device property, probe or switch: the change is the same on every device, and devices without native f16 conversions keep the software path unchanged.Nothing else moves: no guard branches (#5057's lesson), the NaN rule itself, the scalar paths and the f16 <-> f32 conversion probe of #11351 are untouched (the scalar
v_fma_f16keeps the host NaN when a source is NaN, so its payloads are used).Where the f16 special-value words are (measured first)
On
mainfdb3925940, 79 compute requests of Grand Theft Auto V (agc_shader_replay --spv, RTX 4090 target, 1,835,101 words). Each rule was removed alone with a local, non-exact knock-out, to size it:packedFloat16, source pick +0xfe00)0xfe00on an invalid operation (writeF16)FP16_OVFLThe rules themselves are already short selects on the raw bits; I found no exact rewrite with fewer instructions (a SIMD-within-a-register version of the packed rule costs more than two per-lane selects). The words this PR removes are the NaN rebuilds of the conversions around the packed rule, which the measurement above does not count.
Tested
Windows 11 Pro, MinGW-w64 GCC 15.2.0 (WinLibs r7), Ninja, Release,
BUILD_TESTING=ON,ANYPS5_ENABLE_SPIRV_TOOLS=ON(spirv-val on every module), onmain4e43321bd6(the size and timing numbers below were taken onfdb3925940; nothing undercore/shader/recompilerchanged between the two).New test
agc_driver_float16_unused_nan_payloads(tests/execution/Float16UnusedNanPayloads.cpp): each program is compiled twice, for the device and for a software target (no Float16,float16DenormPreserveoff, where the flags change nothing and every conversion keeps RDNA's NaN payload, as onmain). All results are compared bit for bit:agc_driver_packed_float16_native(add, mul, fma, fmac,op_sel,neg,clamp), each source in turn taking all 65,536 halves while the others are drawn from 26 special values (quiet and signalling NaNs of both signs and several payloads, infinities, zeros, denormals, largest finite) or random halves, under 9 float modes (DX10_CLAMP,IEEE_MODE,FP16_OVFLcombinations, flushed f16 denormals);tests/executionsource (the glob inlibSceAgcDriver/CMakeLists.txt), asagc_driver_float16_unused_nan_payloads. On a device without native f16 conversions (lavapipe, per perf(shader): keep native f16 conversion NaN results when a startup probe matches RDNA #11351's RADV/lavapipe run) it reports a skip.It also checks that a
DX10_CLAMPpacked program has noOpIsNanleft (the payload rebuild of the widening), which fails onmain. A mutation that makes the packed lane lose its NaN-ness (0x7c00instead of0x7e00) fails the comparison at once.Focused tests,
main4e43321bd6(with the new test file added) vs this PR (103cf586a9),ctest -R "^agc.*(f16|float16|pack|fma|minmax|med3|clamp)" -E "^agc_driver_bda_device$" -V,VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationwithVK_LOADER_DEBUG=layer(69Insert instance layer "VK_LAYER_KHRONOS_validation"lines per run, nowas not found but was requested by env var VK_INSTANCE_LAYERS),ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1:mainVUID-lines (main / PR)The 2 Intel failures are the same on both (
agc_driver_packed_unorm4444_partial_write,agc_driver_packed_unorm4444_multisample_partial_write); the skips areagc_driver_f64_fma_subnormals(no Float64),agc_driver_float16_dot(no dot2) andagc_driver_image_sample_lod_clamp.agc_driver_packed_float16_native,agc_driver_nan_quieting,agc_driver_fp16_overflowand the f16 denormal tests pass on both.Full agc suite on the RTX 4090, on
fdb3925940before the rebase (ctest -R "^agc" -E "^agc_driver_bda_device$" -j12): 1065 of 1067 passed; the 2 failures (agc_host_import_cut_lifetime,agc_driver_depth_plane_partial_store) also fail onmain.Size, Grand Theft Auto V compute shaders (the 79 requests of perf(shader): compute v_pk_fma_f16 and v_pk_fmac_f16 with OpFmaKHR #11265,
agc_shader_replay --spv, RTX 4090 target):main0x25b3908000x25b393000(RCAS)The other 77 shaders have no packed f16 math computed in f32 and do not change. The UHD 770 has the same native f16 conversions, so the same programs shrink there.
Pipeline creation, NVIDIA 616.92: the two pipelines that change were captured from a cold Grand Theft Auto V run of each build (
APS5_DUMP_COMPUTE_PIPELINES,APS5_DUMP_COMPUTE_MIN_WORDS=50000, emptyANYPS5_SHADER_CACHE_DIR; the two other captured pipelines are identical in both runs) and compiled withagc_compute_pipeline_replay, 5 times each, each capture salted with a distinctOpSourceExtensionso the driver's own cache misses:| pipeline | words |
pipeline_createms (5 runs) | median ||---|---|---|---|
|
0x25b390800,main| 551,841 | 5696 5728 5753 5748 5746 | 5746 ||
0x25b390800, this PR | 512,714 | 4995 5093 5065 5128 5061 | 5065 (-11.9 %) ||
0x25b393000,main| 97,303 | 590 596 597 586 592 | 592 ||
0x25b393000, this PR | 92,485 | 557 560 564 553 560 | 560 (-5.4 %) |0x25b390800is compiled when the open world first loads, on the shader preparation thread.In game (performance form),
mainfdb3925940vs this PR on it (468733630d, the same change before the rebase), RTX 4090: the open-world load from the same save, one run per build, emptyANYPS5_SHADER_CACHE_DIR,APS5_TRACE_SECONDS=40:80, same machine, both after the capture runs above. Traces, summaries, digests (from currentmain'strace_digest.py) and the replay results: fork release perf-f16-special-values-20261026.main: seconds 40:80 from present 447: 2139 presents, frame p50 17.4 / p90 45.3 / p99 265.0 / max 2395.0 ms, GPU busy 6.0 ms per presentVulkan.ComputePipelineslice in either window: the capture runs had already compiled both0x25b390800modules on this machine, and the NVIDIA driver serves them from its own cache (whichANYPS5_SHADER_CACHE_DIRdoes not reach, and which I don't clear). The compile-time gain shows on a module's first compile per driver, which the salted replays measure; as in perf(shader): compute v_pk_fma_f16 and v_pk_fmac_f16 with OpFmaKHR #11265, this pair checks that nothing else moves.{"commit":"fdb3925940c1","dirty":false,"env":{"APS5_MIMALLOC":"1","APS5_TRACE_DIR":"","APS5_TRACE_SECONDS":"40:80"},"gpu":"NVIDIA GeForce RTX 4090","driver":"NVIDIA 616.92","os":"Windows 10.0.26200","cpu":"13th Gen Intel(R) Core(TM) i9-13900K","cpu_threads":32,"memory_mib":65295,"title_id":"PPSA04264","title":"Grand Theft Auto V","content_version":"01.005.000","resolution":"3840x2160","window":{"spec":"seconds 40:80","first_present":447,"ms":80122.125},"presents":2139,"frame_ms":{"p50":17.384,"p90":45.289,"p99":265.010,"max":2395.008},"gpu_busy_ms_per_present":6.003,"worker_busy_pct":{"queue worker 0x0":73.335,"queue worker 0x28":8.183,"queue worker 0x20":13.191,"queue worker 0x38":13.050,"queue worker 0x30":5.452},"top_phases_ms_per_frame":{"Driver.Packet":41.413,"VideoOut.Flip":16.342,"Driver.Packet.memory_wait":14.905,"Graphics.Draw":13.322,"Driver.Packet.draw":12.134,"Driver.Draw":12.039,"Graphics.Draw.draw_and_resource_release":10.873,"Driver.Packet.rendering_wait":8.780},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":53.397,"events_lost":874828,"totals_full":false},"draw committer":{"events_full_after_s":65.989,"events_lost":320641,"totals_full":false}},"trace":"aps5-trace-fdb3925940c1-28652.json.gz"}
{"commit":"468733630da2","dirty":false,"env":{"APS5_MIMALLOC":"1","APS5_TRACE_DIR":"","APS5_TRACE_SECONDS":"40:80"},"gpu":"NVIDIA GeForce RTX 4090","driver":"NVIDIA 616.92","os":"Windows 10.0.26200","cpu":"13th Gen Intel(R) Core(TM) i9-13900K","cpu_threads":32,"memory_mib":65295,"title_id":"PPSA04264","title":"Grand Theft Auto V","content_version":"01.005.000","resolution":"3840x2160","window":{"spec":"seconds 40:80","first_present":445,"ms":80078.358},"presents":2131,"frame_ms":{"p50":17.137,"p90":44.708,"p99":251.680,"max":2630.836},"gpu_busy_ms_per_present":6.299,"worker_busy_pct":{"queue worker 0x0":73.516,"queue worker 0x38":8.786,"queue worker 0x30":5.217,"queue worker 0x20":13.877,"queue worker 0x28":11.596},"top_phases_ms_per_frame":{"Driver.Packet":41.576,"VideoOut.Flip":16.862,"Driver.Packet.memory_wait":14.848,"Graphics.Draw":13.503,"Driver.Packet.draw":12.181,"Driver.Draw":12.088,"VideoOut.Flip.vblank_wait":11.391,"Graphics.Draw.draw_and_resource_release":11.052},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":56.086,"events_lost":858961,"totals_full":false},"draw committer":{"events_full_after_s":66.367,"events_lost":307778,"totals_full":false}},"trace":"aps5-trace-468733630da2-30284.json.gz"}