Repository navigation
fix(shader): declare f16 and f64 DenormPreserve together when their modes are shared - #11483
tibzejoker wants to merge 2 commits into
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I checked the float-control gating and paired emission, the recompile cache key, and the serializer’s format-gated backward compatibility. The submitted Windows and RADV results are detailed, and the automated policy checks pass. The branch is 87 commits behind current (merge base ); CONTRIBUTING requires keeping branches current. Please rebase onto current , resolve the serializer-version interaction with open #11351 if needed, and rerun the focused tests on the rebased head. I’ll review the new head.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I checked the float-control gating and paired DenormPreserve emission, the recompile cache key, and the serializer’s format-gated backward compatibility. The submitted Windows and RADV results are detailed, and the automated policy checks pass. The branch is 87 commits behind current upstream/main (merge base e476a8a); CONTRIBUTING requires keeping branches current. Please rebase onto current main, resolve the serializer-version interaction with open #11351 if needed, and rerun the focused tests on the rebased head. I’ll review the new head.
95a385d to
406bf3f
Compare
|
@MarcZX10 rebased: head
|
|
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.
|
|
On the overlap table: these PRs all bump the request serializer format, so they edit the same version pins in |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Correction to my earlier review: its formatting omitted the branch refs. I fetched the current PR head 406bf3fdc2d5 and verified it is 2 commits ahead and 0 behind current upstream/main (48c160c58b58); the merge is clean. The stale-base concern is resolved.
Thanks for clarifying the serializer overlap with #11351. I agree those are independent changes that only need sequential serializer version numbers, so Depends on: none is appropriate.
I reviewed the coupled-mode device gating, cache-key and request serialization propagation, and paired SPIR-V execution-mode emission. The Vulkan rule for 32_BIT_ONLY requires identical denormal modes for 16-bit and 64-bit types; this change emits both when the device supports both and the shader uses either. The current policy checks pass. The PR reports focused NVIDIA results and a separate RADV run; I did not rerun the GPU tests locally.
|
Thanks @MarcZX10 for the review and for checking the branch freshness. |
THEMANnt
left a comment
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed 406bf3fdc2d5 against 48c160c58b58. No actionable findings.
The device gate requires Float64, Vulkan 32_BIT_ONLY, and preserve support for both widths. Either backend path that emits DenormPreserve also emits the paired 16/64 mode. The coupled flag is carried through the cache key and request format 47; older format 46 requests default it to false.
Validation: GCC 15.3.0, CMake 4.3.3 and Ninja 1.13.2 built libSceAgcDriver, shader_recompiler_agc_driver, the serializer/f16/f64 tests and agc_shader_replay. Focused CTest passed 4/4; replay fixture tests passed 2/2. Both replay mode switches materialized the fixture. A review-only cache-key assertion passed; removing the cache-key append made it fail as expected. On the exact-head Actions run, eligibility passed while Windows, Linux, relinker, ASan, register-context and write-watch jobs were skipped.
The local build used a cached base integration (035ac96) with the PR source overlay, not a full checkout of the pinned base/head pair. No local 32_BIT_ONLY GPU was exercised; the PR's RADV results remain author-reported.
|
More RADV coverage for the coupling, from @tablefrogpillow on #11351 (comment), on the switched version of this change, Mesa 25.2.8, validation layer on:
|
|
@tibzejoker, the 860M run of this PR itself, without any variable: head On RADV the coupling turns on by itself. Every device creation in the focused set logs this line, 74 per run; on The focused set is the one in the description:
The branch now conflicts with Logs of every run above, with an index in AI assisted: yes |
|
A run on the case Tested lists as not covered: AMD's Windows driver. Windows 11 Home 10.0.26200, WinLibs GCC 15.2.0 r7 (MinGW-w64 UCRT), Release,
This head now conflicts with
The layer was active on both sides: each log has 19 Not run: the full AI-assisted: yes (Claude Code) |
406bf3f to
5a8a216
Compare
|
Rebased on Focused set from the description with @Lander0606 @ycbuzi thanks for the RADV 860M and AMD Windows runs. AI-assisted: yes (Claude Code) |
|
Same AMD Windows comparison on the rebased head
The layer check before each side wrote a 28-line log, as before. The serializer format change to 49 doesn't change anything here. AI-assisted: yes (Claude Code) |
|
@tibzejoker this no longer merges into main: Note: #10547, #11483 and #10846 all need the next request-serializer version, so whichever lands first takes 50 and each later one takes the next number after it. |
|
Rebased on Focused set from the description, with
The UHD 770 failure is AI-assisted: yes (Claude Code) |
github-actions
Bot
added
needs rebase
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — changes requested
What I checked
- Reviewed exact head
b81b8a12ff1e0b0ccaae3c7e11baebaac54b5e18against fetchedmain2960e9a6e7. - The author reports exact-head SPIR-V, serializer, and paired GPU tests on the earlier pinned baseline, including RADV validation-layer results. Hosted Windows, relinker, conventions, and eligibility checks pass; product Linux and specialized jobs were skipped. I did not run GPU tests locally.
- The current-main merge preview conflicts in
ShaderReplay.cpp. The pairwise preview with open #11351 conflicts inShaderMemory.cpp,ShaderReplay.cpp,RequestSerializer.cpp, andRequestSerializerTests.cpp.
Blocking integration items
The branch's latest tested main is 319621ba2e, 232 commits behind fetched current main. Please refresh it onto current main, resolve the reported conflicts, and rerun the focused shader/SPIR-V and serializer tests on that integration head. The PR and #11351 both use the request-format version slot; agree which one lands first and record the corresponding Depends on order/version update in both PRs as required by CONTRIBUTING.md.
The older test results are useful evidence, but they do not validate the current conflicting integration. I cannot renew the earlier approval on this head until the rebase, order, and focused rerun are complete.
|
@tibzejoker The version-50 update addresses the earlier collision with main's version-49 depth-export layout. Please also rebase current main: the trial merge conflicts in core/libs/prx/libSceAgcDriver/tools/ShaderReplay.cpp around the per-request target overrides. Preserve main's replay/refactoring behavior and apply the two coupled-denormal switches in the appropriate request path. Keep version-49 depth bounds and legacy decoding intact, and rerun request_serializer, agc_shader_memory and both focused f16/f64 SPIR-V coupling checks on the refreshed head. |
b81b8a1 to
df510d2
Compare
|
@oneandonlydean Rebased on current
Serializer: unchanged. Version 49 depth bounds, legacy decoding and the format-50 tail are as before; Tests on
Landing order. On #11351, adriadam10 proposes that #11351 lands first with format 50 and this PR then takes 51. I agree. I'll renumber this PR to 51 (serializer, the two test files, the version-52 rejection) as soon as #11351 is in AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — changes requested
The update resolves the earlier serializer/replay concerns and provides current-head Vulkan validation runs on an RTX 4090 and UHD 770, with the layer confirmed loaded and no new VUID- lines. Current main is now ba5b5330b3; the PR head df510d2219 does not include it, and the intervening change touches the shader debt file this PR edits. Please integrate current main and rerun conventions on the resulting tree. Since the report says the intervening change is documentation-only and the tested source is unchanged, the existing GPU results can stand if the refreshed diff confirms that. Keep the declared stack ordering intact.
…odes are shared A device that reports VK_SHADER_FLOAT_CONTROLS_INDEPENDENCE_32_BIT_ONLY must use the same denormal execution mode for 16-bit and 64-bit floats in an entry point. The backend only preserved f16 and f64 denormals with independence ALL, so on such a device (RADV) f64 denormals were left to the driver and f16 to f32 conversions were emulated, while the native f16 conversions still declared DenormPreserve 16 alone. When the device also has shaderDenormPreserveFloat16, shaderDenormPreserveFloat64 and Float64, both are now preserved and every module that declares one of the two modes declares the other. RDNA keeps one denormal field for f16 and f64 in FLOAT_MODE too. The flag is in the target, the cache key and recompile requests (serializer format 50). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PHJtpJ4hw3KWJhobipK8ia
agc_shader_replay --unpreserved-f16-f64-denorms clears float16DenormPreserve and float64DenormPreserve, as on a 32_BIT_ONLY device before the coupling; --coupled-f16-f64-denorms sets both with coupledF16F64Denorms, as on such a device now. Requests saved on another GPU can then show both module shapes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PHJtpJ4hw3KWJhobipK8ia
df510d2 to
80cb2cd
Compare
|
@MarcZX10 Rebased on current Refreshed diff:
I reran the focused set on
The UHD 770 failure is the same as on AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The rebased head resolves the current-main integration finding, preserves the serializer/replay changes, and has refreshed author-reported GPU results with no new validation IDs.
What I ran
- Exact PR head:
80cb2cd3acfdcde0abd374bc3f8886b5bd23e092. - Current base:
main420b6a064eadaa7c5e50b080c7fad59c41150b4b; merge preview is clean (97f6e7e7826a2d278e1bad3d108f26170528ca44). - The prior reported test base
184afa49adce16e298c47d136706fd2356198852differs from currentmainonly in five unrelated relinker/usage paths; none of the PR's 17 changed paths changed in between. - Environment: Linux 6.1.0-54-amd64 x86_64, Git 2.39.5, Python 3.11.2. I did not run a local C++ build or GPU tests; available disk space is insufficient for a build.
- Commands:
git merge-tree --write-tree upstream/main refs/remotes/review/pr-11483
git diff --check upstream/main...refs/remotes/review/pr-11483
python3 tools/check_conventions.py --base upstream/main --head refs/remotes/review/pr-11483
git diff --name-status 184afa49adce16e298c47d136706fd2356198852 upstream/main -- <the 17 PR paths>Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean; tree 97f6e7e7826a2d278e1bad3d108f26170528ca44. |
git diff --check |
Passed; no output. |
| Conventions | Passed. |
| Intervening-main path comparison | None of the PR's changed paths changed between the reported test base and current main. |
| Author-reported focused GPU tests | Exact head 80cb2cd3ac: RTX 4090, 89/89; UHD 770, 88/89 with the same agc_driver_packed_unorm4444_multisample_partial_write failure on main; 0 VUID- lines on either. request_serializer, agc_shader_memory, and the coupled f16/f64 checks passed on both GPUs. |
| Hosted checks | Windows, Ubuntu relinker, relinker ASan, eligibility, conventions, labels and report pass; Linux product job is skipped; two macOS relinker jobs remain queued. |
Unedited local output
$ git merge-tree --write-tree upstream/main refs/remotes/review/pr-11483
97f6e7e7826a2d278e1bad3d108f26170528ca44
$ python3 tools/check_conventions.py --base upstream/main --head refs/remotes/review/pr-11483
All conventions checks passed.
$ git diff --name-status 184afa49adce16e298c47d136706fd2356198852 upstream/main -- <the 17 PR paths>
(no output)
Negative control
| Assertion | Change to code under test | Did the new test fail? |
|---|---|---|
| Paired baseline for the focused serializer/SPIR-V behavior | Author ran unchanged main 184afa49adce and the PR head on each GPU; this is a baseline comparison, not a mutation. |
No new failure reported; UHD 770's one failure is reported on both trees. No mutation control was run. |
Findings
Blocking
None found on this exact head.
Non-blocking
#11351 remains open and also currently targets serializer format 50. The PR description records the agreed order: if #11351 lands first, this PR will move to 51 and rerun the focused set. Recheck that exact changed head before approval of any subsequent revision.
Sources
Not verified
I did not build or run the C++ tests locally, independently repeat the NVIDIA/Intel runs, test the console, or wait for the queued macOS jobs. The GPU results above are author-reported.
To get this to a good state
No change is requested on the reviewed head. Keep the stated serializer landing order; if #11351 lands first and this PR moves to format 51, I will review and rerun the focused commands on that new head.
✅ 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 (4, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (5 IDs in 476 tests)Skipped (44)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. |
|
Another device for this PR: savvaniss ran #11483 on an RX 9070 XT (RDNA 4, AMD Windows driver 26.8.1,
That adds RDNA 4 on AMD's own driver to the earlier iGPU and RADV results. AI-assisted: yes (Claude Code) |
XairRrR
left a comment
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
This pull request does not merge cleanly into main (4f41f4c): head 80cb2cd conflicts in
core/libs/prx/libSceAgcDriver/tests/ShaderMemory.cppcore/shader/recompiler/ControlFlow/src/RequestSerializer.cppcore/shader/recompiler/tests/RequestSerializerTests.cppdocs/dev/debt/shader.md
Please rebase it onto current main. I will review the change once it merges cleanly.
|
The conflicts are the request format. adriadam10 agreed on the landing order on #11351: #11351 goes first, then this PR rebases and takes the next number. AI-assisted: yes (Claude Code) |
XairRrR
left a comment
XairRrR
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
This pull request does not merge cleanly into main (1e65852): head 80cb2cd conflicts in
core/libs/prx/libSceAgcDriver/tests/ShaderMemory.cppcore/shader/recompiler/ControlFlow/src/RequestSerializer.cppcore/shader/recompiler/tests/RequestSerializerTests.cppdocs/dev/debt/shader.md
Please rebase it onto current main. I will review the change once it merges cleanly.
What
Part of #11183. On a device whose f16 and f64 denormal modes are tied together, both are now preserved, and every module that declares
DenormPreservefor one of the two widths declares it for the other.Vulkan's
denormBehaviorIndependence=VK_SHADER_FLOAT_CONTROLS_INDEPENDENCE_32_BIT_ONLYmeans an entry point must use the same denormal execution mode for 16-bit and 64-bit floats (VkShaderFloatControlsIndependence). #2977 enabledDenormPreserve64 only with independenceALLand noted that "the backend does not yet emit coupled execution modes for32_BIT_ONLYorNONE".float16DenormPreservehas the sameALLcondition. RADV reports32_BIT_ONLY, so on RADV today:DenormPreserve64), andagc_driver_f64_fma_subnormalschecks the result only where the driver keeps them;v_cvt_f32_f16and the other f16 -> f32 reads use the decoded integer emulation instead ofUnpackHalf2x16, because that path needsfloat16DenormPreserve;DenormPreserve16 by themselves (their condition,Float16withRoundingModeRTEandDenormPreserve16 supported, does not look at the independence), so a module with both those conversions and f64 code declares a 16-bit mode with no 64-bit one. None of the 78 GTA V compute modules below has both.RDNA has a single f16/f64 denormal field in
FLOAT_MODE(bits 7:6), so preserving both together is also the console's shape.Change:
VulkanDevice: with Float64 enabled,32_BIT_ONLY,shaderDenormPreserveFloat16andshaderDenormPreserveFloat64, the device setsSpirvTarget::coupledF16F64Denorms,float16DenormPreserveandfloat64DenormPreserve, and logs[float-controls] f16 and f64 denormals share one mode on this device: DenormPreserve 16 and 64 are declared together. With independenceALLorNONE, or a missing preserve property, nothing changes.EmitF16DenormPreserveand the newEmitF64DenormPreserveeach declare the other when the flag is set. They are the only places that emit the 16-bit and 64-bitDenormPreservemodes (git grep ExecutionModeDenorm), so every module declares both or neither.agc_shader_replay --unpreserved-f16-f64-denorms/--coupled-f16-f64-denormsreplay a saved request in the two shapes, so the size numbers below can be reproduced from requests saved on any GPU.Landing order with #11351 (agreed on #11351): both append to the request format.
mainhas since taken formats 51, 52 and 53 and rejects 50, so #11351 now lands with 54; this PR then rebases and takes 55, keeping both flags.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), onmain48c160c58b.SPIR-V checks (no device needed):
agc_driver_f64_denorm_preserve_spirv: an f64 shader declaresDenormPreserve64 and not 16 with independent modes, and both withcoupledF16F64Denorms;agc_float16_conversion: a native f16 conversion declaresDenormPreserve16 and not 64 with independent modes, and both when coupled.With the two coupling lines removed from
SpirvModuleEmitter.cpp, both tests fail; with them, both pass.Serializer:
request_serializerround-trips the flag both ways, reads a format 49 request as false and rejects a truncated one;agc_shader_memorypins format 50.Focused tests,
main48c160c58bvs this PR (406bf3fdc2),ctest -R "(^agc.*(f16|float16|f64|convert|pack|interpolation|denorm)|request_serializer|shader_memory|agc_float16_conversion)" -E "^agc_driver_bda_device$" -V,VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationwithVK_LOADER_DEBUG=layer(75Insert 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-linesNeither device reports
32_BIT_ONLY, so neither takes the new path: this run is the no-change control. On Intel the f64 tests skip (no Float64), and the 2 failures areagc_driver_packed_unorm4444_partial_writeandagc_driver_packed_unorm4444_multisample_partial_write, on both trees.RADV, Radeon 860M (Mesa 26.2.5, Fedora 44, GCC 13.3), by @Lander0606, on the same change behind a switch (
test/coupled-f16-f64-denormsfe10c0205c, on top of perf(shader): keep native f16 conversion NaN results when a startup probe matches RDNA #11351), validation layer 1.4.341,ctest -j1 -V -R "^agc.*(f16|float16|f64|convert|pack|interpolation)" -E "^agc_driver_bda_device$":^agc(-j8)Every device creation logs the
share one modeline. The 13 f64 tests of the set pass withDenormPreserve64 on (f64_fma_subnormals,f64_rounding_denormal_modes,div_fixup_f64_denormal_modes, ...), and so dointerpolation_modesandpixel_exec_interpolation. The only full-suite failure isagc_driver_tessellation(the Mesa 26.2.5 crash), on both. NoVUID-line, but @Lander0606 also checked that the layer does not flag an uncoupled declaration (VUID-RuntimeSpirv-denormBehaviorIndependence-06289stayed silent with the f64 half removed), which is why the two SPIR-V checks above test the coupling directly. On lavapipe (denormBehaviorIndependencenot32_BIT_ONLY) the flag stays off. A rerun on this branch is offered.Size, Grand Theft Auto V compute shaders (the requests of perf(shader): compute v_pk_fma_f16 and v_pk_fmac_f16 with OpFmaKHR #11265 saved on the RTX 4090, replayed with
agc_shader_replay --spv; 78 of them recompile on currentmain):--unpreserved-f16-f64-denorms(a32_BIT_ONLYdevice onmain)--coupled-f16-f64-denorms(the same device with this PR)ALL, unchanged by this PR)0x25b393000(RCAS) 102,227 -> 90,517 (-11.5 %),0x25b433400112,824 -> 110,242 (-2.3 %),0x25b3c4e00-0.6 %,0x25b3c3a00-1.0 %; modules that change nothing else gain the 4 words of the second mode. Both coupled modules checked withspirv-val --target-env vulkan1.3(0x25b393000: 16 and 64;0x25b433400, which has Float64: 64 and 16).What is and is not measured. The path is taken only on
32_BIT_ONLYdevices (RADV here). Measured: module shape, size and test correctness on RADV. Not measured: frame time or pipeline creation on such a device (no title runs on the 860M). Not tested: AMD's Windows driver, MoltenVK.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
https://claude.ai/code/session_01PHJtpJ4hw3KWJhobipK8ia