Repository navigation
fix(shader): emit OpFmaKHR for f32 fma when VK_KHR_shader_fma is available - #15343
Open
tablefrogpillow wants to merge 2 commits into
Open
tablefrogpillow wants to merge 2 commits into
tablefrogpillow wants to merge 2 commits into
Conversation
BunnyFl
approved these changes
Nov 3, 2026
BunnyFl
left a comment
BunnyFl
left a comment
Contributor
There was a problem hiding this comment.
Windows 11, AMD RX 9070 XT (driver reports VK_KHR_shader_fma revision 1), WinLibs GCC, ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, merged onto current main:
- 12 f32 FMA, denormal, NaN, clamp and output-modifier tests pass (
agc_driver_fma_rounding,_fma_legacy,_f32_fma_denormal_mode2,_f32_arithmetic_nan,_f32_arithmetic_nan_fast,_f32_denormal_flush,_dx10_clamp,_f32_output_modifier,_f32_result_clamp,_f32_constant_factor_tininess,_f32_add_mul_denormal_modes,_mullit_f32_denormal_modes). With main's two files restored they pass too; nothing changed in these results. - To confirm the new branch ran: with a
throwadded afterEmitExtension("SPV_KHR_fma")inEmitFPFma32,agc_driver_fma_roundingfails with that message.
Non-blocking: core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp:1617 adds "SPV_KHR_shader_fma" to spirvExtensions, but the SPIR-V extension is SPV_KHR_fma, which is the name the emitter declares (core/shader/recompiler/SpirvBackend/src/SpirvAlu/SpirvAluEmitterMath.cpp:1938). 3rdparty/SPIRV-Headers/include/spirv/unified1/spirv.core.grammar.json:18223 lists FMAKHR under SPV_KHR_fma. On this head the two lines above are the only occurrences of either name under core/, and EmitFPFma32 checks CapabilityFMAKHR, not the extension list; the list also goes into the feature digest (core/shader/recompiler/CacheKey.hpp:72). It's worth correcting before something gates on it.
Collaborator
|
Hi tablefrogpillow, Thanks for this; the fallback keeps drivers without the extension unchanged, and the Vega result shows the split being avoided. One small fix before merge, the same one raised in the earlier review: |
Contributor
✅ AMD GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (8, not counted against this PR)Validation IDs already present on main (4 IDs in 525 tests)Skipped (58)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
MarcZX10
reviewed
Nov 3, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — no blocking findings
The device feature query, enable chain and FMA emission are consistent with the reported GPU behavior, but a SPIR-V extension-name typo remains and two hosted macOS checks are pending, so I am withholding approval.
What I ran
Reviewed both changed files at head bd74e5e4ee685eb8c39fb3b066b075f0855bfce1 against base 7feb37ee3e30e10139b483fb4df689d5b5b69ab8; the current-base merge preview is clean. Environment: Debian Linux x86_64, Python 3.11.2, GCC 12.2.0. I did not build, compile, or run the GPU tests locally. The PR description and a separate review report targeted GPU test results; those are reported evidence, not tests I ran.
git diff --check refs/review/main-15343-current...refs/review/15343-current
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 15343 --output-dir /tmp/anyps5-pr-precheck/15343-20261103
gh pr checks 15343 --repo boykopovar/AnyPS5
git grep -n -E 'FMAKHR|SPV_KHR_shader_fma|SPV_KHR_fma|spirvExtensions' refs/review/15343-current -- core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp core/shader/recompiler/SpirvBackend/src/SpirvAlu/SpirvAluEmitterMath.cpp core/shader/recompiler/CacheKey.hpp
git show refs/review/15343-current:core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
git show refs/review/15343-current:core/shader/recompiler/SpirvBackend/src/SpirvAlu/SpirvAluEmitterMath.cppResults
| Check | Result | Note |
|---|---|---|
| Diff and conventions | passed | Exact-head diff and repository conventions passed. |
| Feature and SPIR-V source review | passed | Supported feature gates the extension, capability and instruction. |
| Hosted checks | passed | Windows, Linux relinker, ASan, portability and policy checks passed. |
| Hosted macOS relinker checks | not run | Both architecture jobs remain pending. |
| Optional precheck helpers | failed | ABI and test-quality scripts are absent from the isolated PR snapshot. |
| Local build and GPU tests | not run | These require compilation and a suitable Vulkan device. |
| Reported focused GPU tests | passed | Author and another reviewer report focused tests; not independently reproduced here. |
Full precheck table, unedited precheck output, and hosted checks
| Check | Status | Command / reason |
|---|---|---|
| build_and_compile | not_applicable | The precheck deliberately does not configure, build, or compile AnyPS5. |
| diff_check | passed | git diff --check 7feb37ee3e30e10139b483fb4df689d5b5b69ab8...bd74e5e4ee685eb8c39fb3b066b075f0855bfce1 |
| conventions_and_pr_template | passed | /usr/bin/python3 tools/check_conventions.py --base 7feb37ee3e30e10139b483fb4df689d5b5b69ab8 --head bd74e5e4ee685eb8c39fb3b066b075f0855bfce1 --title fix(shader): emit OpFmaKHR for f32 fma when VK_KHR_shader_fma is available --body /tmp/anyps5-pr-precheck/15343-20261103/pr-body.md |
| changed_export_abi_audit | failed | /usr/bin/python3 tools/pr_abi_audit.py --base 7feb37ee3e30e10139b483fb4df689d5b5b69ab8 --head bd74e5e4ee685eb8c39fb3b066b075f0855bfce1 --json |
| python_syntax | not_applicable | No changed Python implementation files. |
| related_python_unit_tests | not_applicable | No changed tools/ module has an existing sibling tools/tests/test_<module>.py suite. |
| python_test_registration | not_applicable | No changed test_*.py files. |
| live_dependency_and_mergeability_check | passed | /usr/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 15343 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-pr-precheck/15343-20261103/stack |
| build_dependent_cpp_tests | not_applicable | Not run: C++ tests need exact-head binaries, which require a build; the precheck does not build. |
| test_quality_heuristics | failed | /usr/bin/python3 /tmp/anyps5-pr-precheck/15343-20261103/head-bd74e5e4ee68-20fe9b8b/tools/pr_test_quality.py --root /tmp/anyps5-pr-precheck/15343-20261103/head-bd74e5e4ee68-20fe9b8b --json core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp core/shader/recompiler/SpirvBackend/src/SpirvAlu/SpirvAluEmitterMath.cpp |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
conventions_and_pr_template — stdout
All conventions checks passed.
conventions_and_pr_template — stderr
changed_export_abi_audit — stdout
changed_export_abi_audit — stderr
/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck/15343-20261103/head-bd74e5e4ee68-20fe9b8b/tools/pr_abi_audit.py': [Errno 2] No such file or directory
live_dependency_and_mergeability_check — stdout
/tmp/anyps5-pr-precheck/15343-20261103/stack/stack-report.md
/tmp/anyps5-pr-precheck/15343-20261103/stack/stack-report.json
live_dependency_and_mergeability_check — stderr
test_quality_heuristics — stdout
test_quality_heuristics — stderr
/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck/15343-20261103/head-bd74e5e4ee68-20fe9b8b/tools/pr_test_quality.py': [Errno 2] No such file or directory
Unedited gh pr checks output:
edge-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41562596817/job/125682093809
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41562596817/job/125682093704
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41562596817/job/125682093473
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41562596817/job/125682094134
conventions pass 21s https://github.com/boykopovar/AnyPS5/actions/runs/41562596782/job/125676445026
intake pass 23s https://github.com/boykopovar/AnyPS5/actions/runs/41562596830/job/125676444697
labels pass 10s https://github.com/boykopovar/AnyPS5/actions/runs/41562596727/job/125676443913
relinker (ubuntu-latest, g++) pass 4m41s https://github.com/boykopovar/AnyPS5/actions/runs/41562596817/job/125682092294
relinker (windows-latest, g++) pass 7m25s https://github.com/boykopovar/AnyPS5/actions/runs/41562596817/job/125682092409
relinker-asan pass 1m50s https://github.com/boykopovar/AnyPS5/actions/runs/41562596817/job/125682092296
report pass 58s https://github.com/boykopovar/AnyPS5/actions/runs/41562597274/job/125676444443
windows pass 4m1s https://github.com/boykopovar/AnyPS5/actions/runs/41562596817/job/125682092284
relinker (macos-latest, clang++) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41562596817/job/125682092369
relinker (macos-latest, clang++, x86_64) pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41562596817/job/125682092357
eligibility pass 7s https://github.com/boykopovar/AnyPS5/actions/runs/41562596817/job/125676442730
library-portability pass 1m42s https://github.com/boykopovar/AnyPS5/actions/runs/41562597190/job/125676444173
Negative control
I did not run a negative control locally because the focused C++ tests require a build. A separate reviewer reports that inserting a throw after the new emitter's EmitExtension("SPV_KHR_fma") makes agc_driver_fma_rounding fail, which is reported evidence rather than my own run.
Findings
Blocking
None found in the two changed files.
Non-blocking
VulkanDevice.cpp adds "SPV_KHR_shader_fma" to spirvExtensions, but the SPIR-V extension name is SPV_KHR_fma, which the emitter actually declares. The current FMA path checks CapabilityFMAKHR, so this mismatch does not block the present execution path; correcting the advertised extension name keeps the target's supported-extension metadata accurate.
state->spirvExtensions.push_back("SPV_KHR_fma");
Sources
- Vulkan feature query and extension metadata at this head
- FMA capability gate and emitted extension at this head
- Khronos SPIR-V FMA extension specification at a pinned commit
Not verified
I did not independently reproduce the author's Linux RX Vega 56/RX 7900 GRE results or the other reviewer's Windows RX 9070 XT results. The two macOS relinker jobs are pending. No local build, CTest or console run was performed.
To get this to a good state
No blocking code change is required for this review. Correct the non-blocking extension-name typo when convenient; I will recheck this head and the same hosted checks before considering approval once both macOS jobs complete.
Contributor
❌ NVIDIA GPU test: failedPR head
Failing tests
agc_driver_tessellation_depth_range_remap: Failed (also failed when re-run alone), assertion and diagnosticsagc_driver_tessellation_geometry: Failed (also failed when re-run alone), assertion and diagnosticsNew validation errors
…truncated; see the full logs. Automated 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. |
tablefrogpillow
force-pushed
the
fix/fpfma32-opfmakhr-15316
branch
from
November 3, 2026 09:56
bd74e5e to
a99a473
Compare
Contributor
Author
|
Thanks — pushed, and I fixed the Heads-up on the NVIDIA GPU-CI failure that came in: on the RTX 2000 Ada run the device advertises Since the Re-validated on RADV with the gate: Vega 56 (VEGA10) still flips FAIL→PASS with fused If you'd rather not carry a driver gate, I'm happy to adjust — let me know how you'd prefer an advertised-but-unusable |
Contributor
✅ NVIDIA GPU test: passedPR head
Failing on main too (3, not counted against this PR)Validation IDs already present on main (4 IDs in 525 tests)Skipped (49)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. |
EmitFPFma32 emitted GLSLstd450Fma, whose precision is inherited from OpFMul then OpFAdd. Mesa lowers that to a split fmul+fadd on RADV before RDNA2, rounding the product before the add, so agc_driver_f32_fma_denormal_mode2 returns 0x80000001 instead of the fused 0x80000002 on GCN. EmitFPFma32 now emits OpFmaKHR (correctly rounded) when the FMAKHR capability is available, matching the f16 path. VulkanDevice detects shaderFmaFloat32 alongside shaderFmaFloat16, enables VK_KHR_shader_fma and the FMAKHR capability when either is supported, and requests only the feature bits the device reports. Fixes boykopovar#15316 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tablefrogpillow
force-pushed
the
fix/fpfma32-opfmakhr-15316
branch
from
November 5, 2026 14:09
a99a473 to
f88dea7
Compare
Contributor
Author
|
Rebased onto current Re-validated |
|
A run of the reworked head on NVIDIA with
One question from reading the code, not covered by this run since this device has both bits:
No title was run. AI-assisted: yes. Claude Code ran the builds and tests and drafted this comment; a separate review agent checked it and found the validator point. The numbers are from the runs on this machine. |
The f32 path declares FMAKHR and SPV_KHR_fma when shaderFmaFloat32 is enabled, but ShaderValidation accepted them only when fmaFloat16, so an f32 fma graphics shader on a device without the f16 conditions was refused. Thread fmaFloat32 alongside fmaFloat16 and accept the capability and extension when either is set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Good catch on the Re-validated on RX Vega 56 (RADV VEGA10, Mesa 26.2.6): GCC full build clean, conventions pass, and |
BunnyFl
reviewed
Nov 5, 2026
BunnyFl
left a comment
BunnyFl
left a comment
Contributor
There was a problem hiding this comment.
@tablefrogpillow re-review of 7675de4f9a, after the rework onto #11265's plumbing and the ShaderValidation fix. My approval was on bd74e5e4ee; I'm not re-approving yet because of one question below. cc @adam25136, whose RTX 5080 run found the validator point.
What I ran: this head merged onto main fc23faa40a (merge e92afec3a8, clean), Windows 11 Pro 10.0.26300, WinLibs GCC, Release, AMD Radeon RX 9070 XT, AMD proprietary driver 26.10.2 (vulkaninfo: shaderFmaFloat16, shaderFmaFloat32 and shaderFmaFloat64 all true), with ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation and ctest -V:
| the merge | the merge with SpirvAluEmitterMath.cpp from main |
|
|---|---|---|
agc_driver_f32_fma_denormal_mode2, agc_driver_fma_rounding, agc_driver_fma_legacy, agc_driver_f16_fma_denormal_modes, agc_driver_f16_fma_invalid_nan, agc_driver_float16_fma_rounding, agc_driver_fma_mix_f16_denormal_modes, agc_driver_dpp8_pk_fmac, agc_fma_mix_once, agc_driver_graphics |
10 of 10 pass | 10 of 10 pass |
VUID- ids / layer-not-found lines |
none / 0 | none / 0 |
agc_driver_f32_fma_denormal_mode2 prints 2 of 2 IEEE modes run in both runs. So on this GPU the test passes with main's f32 emitter too, and this run shows no regression but doesn't reproduce your Vega red/green.
Question (potential bug, from reading this head; not run: this GPU reports every fma bit, per vulkaninfo above): the two widths now share one capability.
core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp:1726declaresFMAKHRandSPV_KHR_fmawhenfloat16Fma || float32Fma, while:1722and:1723enableshaderFmaFloat16andshaderFmaFloat32separately.core/shader/recompiler/SpirvBackend/src/SpirvAlu/SpirvAluEmitterMath.cpp:1946:EmitFPFma32emits an f32OpFmaKHRwheneverFMAKHRis among the supported capabilities.core/shader/recompiler/Recompiler.cpp:266:nativeFloat16Fma = nativeFloat16 && supports(spv::CapabilityFMAKHR), which leads to the f16OpFmaKHRinEmitFPFma16x2(SpirvAluEmitterMath.cpp:1886).
On a device that reports shaderFmaFloat16 without shaderFmaFloat32, that would put an f32 OpFmaKHR in a shader on a device created with shaderFmaFloat32 = VK_FALSE. A device with only shaderFmaFloat32 (and the f16 RTE conditions) would get the reverse for f16. On main, FMAKHR is declared only with float16Fma, and f32 fma never uses OpFmaKHR. Is an OpFmaKHR of a width whose feature isn't enabled allowed by VK_KHR_shader_fma? If not, gating each path on its own bit (passing fmaFloat32 to the recompiler target, as the validator now receives it) would keep the two apart. I haven't found a device that reports only one of the two bits.
Overlap (tools/pr_overlap.py --branch on this head): #9508 (perf(agc): GPU descriptor heap) changes the same ShaderValidation.cpp lines on main (L363, the SPIR-V extension check, and L710-711), and neither PR lists the other under Depends on. Whichever lands second will need to redo those lines.
AI-assisted: yes (Claude Code)
Contributor
❌ NVIDIA GPU test: failedPR head
New validation errors
…truncated; see the full logs. Automated 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. |
Collaborator
|
Hi @tablefrogpillow, thanks for fixing the extension name and for the ShaderValidation change. One problem with the new device setup in VulkanDevice.cpp:
Could the f16 path be keyed on the f16 feature, for example a separate target flag or The same question applies to the validation allow-list, which accepts FMAKHR when either flag is set; that is fine once the emitter side is separated. |
Contributor
Author
|
Thanks — I've implemented and validated the keying locally, and have a design question before I push the serialization part. What I have: The question: as |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
FPFma32(guestv_fma_f32/v_fmac_f32/v_fmamk_f32/v_fmaak_f32) emittedGLSLstd450Fma, whose precision is "inherited fromOpFMulfollowed byOpFAdd". Mesa lowers that to a splitfmul+faddon RADV before RDNA2, rounding the product before the add — soagc_driver_f32_fma_denormal_mode2returns0x80000001instead of the fused0x80000002on GCN.EmitFPFma32now emitsOpFmaKHR(correctly rounded) when theFMAKHRcapability is available — the same path #11265 added for packed f16.VulkanDevicedetectsVK_KHR_shader_fma'sshaderFmaFloat32alongsideshaderFmaFloat16, enables the extension and declaresFMAKHR+SPV_KHR_fmawhen either is supported, and requests only the feature bits the device reports. There is no driver gate: the capability is declared only when the device's feature bit is set, which is the signal the recompiler already keys on. On a device without the extension (or the bit), the unchangedGLSLstd450Fmapath is used.Tested
AGC shader recompiler.
agc_driver_f32_fma_denormal_mode2,mainvs this change, same machine and GPU,ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1:main(already carrying perf(shader): compute v_pk_fma_f16 and v_pk_fmac_f16 with OpFmaKHR #11265): FAIL (v_fmamk_f32 ... is 0x80000001, expected 0x80000002). This change: PASS (both IEEE modes run). The flip is produced by this change.Checklist
main; no other open PR implements the same functions.mdfiles or images added to the repository🤖 Generated with Claude Code