Repository navigation
feat(shader): allow directed rounding modes for packed f16 minmax - #14793
AlexWhitehouse wants to merge 2 commits into
Conversation
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — no blocking findings
The directed-rounding exemption is limited to packed min/max opcodes, while other packed arithmetic remains guarded; the added cases exercise the min/max path across the configured mode combinations.
What I ran
Reviewed head 97c53a1d091b3ac085e1adf011485edddc8f6935 against base f2d874c1fdccf781c0fa5d86619bb5dd8894b958; the merge preview is clean. Environment: Linux x86-64, GCC 12.2.0, Python 3.11.2. I reviewed the source diff, test-mode construction, test registration and debt edits. No build or GPU test was run locally.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14793
git diff --check f2d874c1fdccf781c0fa5d86619bb5dd8894b958...97c53a1d091b3ac085e1adf011485edddc8f6935Results
| Check | Result | Note |
|---|---|---|
| Diff and conventions | passed | Two existing debt-length warnings; no convention errors. |
| Source and test review | passed | Only packed min/max bypass the rounding guard; add remains guarded. |
| CTest registration | passed | The existing execution-test glob registers Float16MinMaxBits.cpp. |
| Hosted CI | not run | Six checks are queued on this head. |
| GPU execution and mutation check | not run | The PR reports them passing; I did not reproduce them. |
Raw precheck output
/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14793-97c53a1d091b/report.md
/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-14793-97c53a1d091b/report.json
Conclusion: checks failed
The report's diff check passed. Its convention check passed with these warnings:
docs/dev/debt/shader.md:140: warning: [debt-length] Technical debt entry over 1000 characters: state the gap first (what differs from the console, what is unverified, what throws) and keep only the behaviour it needs: 1160 characters
docs/dev/debt/shader.md:145: warning: [debt-length] Technical debt entry over 1000 characters: state the gap first (what differs from the console, what is unverified, what throws) and keep only the behaviour it needs: 1652 characters
2 findings, 0 errors; GitHub shows at most 10 error annotations, the job summary lists all
The overall precheck could not launch its workspace-only pr_abi_audit.py and pr_test_quality.py helpers from the isolated exact-head checkout. Those helper launch errors are not code or test failures. The live stack check passed.
Negative control
Not run by me. The PR reports that swapping the min/max selection made the test fail with an output mismatch; I did not repeat that mutation.
Findings
Blocking
None found in the reviewed changes.
Non-blocking
None.
Sources
- Rounding guard and bit-based min/max lane path at the reviewed head
- Only the packed min/max opcodes select the exempt IR operations
- Directed-rounding mode matrix and packed-add rejection test
- Execution-test source discovery and target setup
Not verified
The reported RTX 3060 and lavapipe runs, the 696,320 comparisons, the mutation result, and validation-layer output were read from the PR description but not independently reproduced. No console behavior was tested. Hosted checks remain queued.
To get this to a good state
No source change is requested. Once the hosted checks complete, I can recheck this head and approve if they pass.
|
Thanks. Letting One request on the test: Reviewed at |
|
Dropped the packed add throw loop and its stdexcept include in 67a16bf; the min/max checks are unchanged. On an RTX 3060 and lavapipe with the validation layer, main fails at the directed-rounding throw and this head passes 696,320 comparisons, with no VUID errors. |
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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.
|
✅ AMD GPU test: passedPR head
Unstable on main (4, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (9, not counted against this PR)Validation IDs already present on main (5 IDs in 484 tests)Skipped (55)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
left a comment
There was a problem hiding this comment.
Reviewed — no blocking findings
The exemption is sound: v_pk_min_f16 and v_pk_max_f16 select operand bits and never round, so the rounding-mode throw does not apply to them. Approval waits for the macOS relinker jobs, which were queued at review time.
What I ran
- Head
67a16bfa4003957fe0648197946f492e9f3c66d2; basef2d874c1fd(merge base is the same commit). 3 files, +26/-7:FloatInstructions.cpp(1/1),Float16MinMaxBits.cpp(23/4),docs/dev/debt/shader.md(2/2). I read the whole diff. - Checked the call sites:
FPMin32andFPMax32reachpackedFloat16only from the packed min and max opcodes, withquietSnanset (VectorInstructions.cpplines 956-958). That branch takespackU16Lanes(minMaxLane(...))(FloatInstructions.cpplines 61 and 68), which selects 16-bit lanes throughminMaxF16Bitswith no rounding step. The add, multiply and fma opcodes still take the throw. - Read the test diff: packed-only runs in every f16 round mode, crossed with IEEE, DX10 clamp and f16 denormal modes, and special-value rows. I did not rerun it.
- Precheck report at this exact head (head and base unchanged at completion). Its ABI and test-quality rows failed because their helpers are not in the head tree. This PR changes no export.
Results
| Check | Result | Note |
|---|---|---|
| Diff check | passed | |
| Conventions and PR template | passed | |
| Mergeability and dependencies | passed | depends on none |
| Packed min and max rounding exemption | passed | min and max take the bit-selection path with no rounding |
| Test registration | passed | existing registered test |
| Build and runtime tests | not run | no build; GPU runs are author-reported |
| Negative controls | not run | the PR reports a mutation check (author-reported) |
| Hosted CI | not run | all completed jobs passed except one cancelled conventions run; macOS relinker queued |
Commands and unedited precheck output
Commands:
- Precheck:
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14793 --output-dir <scratch>/precheck/14793(report.md in that directory). - Head fetched by URL (objects only):
git fetch https://github.com/boykopovar/AnyPS5.git pull/14793/head. - Call sites:
git grep -n 'packedFloat16(' 67a16bfa4003957fe0648197946f492e9f3c66d2. - Hosted checks:
gh api repos/boykopovar/AnyPS5/commits/67a16bfa4003957fe0648197946f492e9f3c66d2/check-runs.
Precheck table (unedited):
| 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 f2d874c1fdccf781c0fa5d86619bb5dd8894b958...67a16bfa4003957fe0648197946f492e9f3c66d2 |
| conventions_and_pr_template | passed | /marc/Conlyse-VIEJO/.venv/bin/python3 tools/check_conventions.py --base f2d874c1fdccf781c0fa5d86619bb5dd8894b958 --head 67a16bfa4003957fe0648197946f492e9f3c66d2 --title feat(shader): allow directed rounding modes for packed f16 minmax --body /dev/shm/reviewer-run/precheck/14793/pr-body.md |
| changed_export_abi_audit | failed | /marc/Conlyse-VIEJO/.venv/bin/python3 tools/pr_abi_audit.py --base f2d874c1fdccf781c0fa5d86619bb5dd8894b958 --head 67a16bfa4003957fe0648197946f492e9f3c66d2 --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 | /marc/Conlyse-VIEJO/.venv/bin/python3 /marc/AnyPS5/tools/pr_stack_check.py 14793 --repo boykopovar/AnyPS5 --output-dir /dev/shm/reviewer-run/precheck/14793/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 | /marc/Conlyse-VIEJO/.venv/bin/python3 /dev/shm/reviewer-run/precheck/14793/head-67a16bfa4003-785579a1/tools/pr_test_quality.py --root /dev/shm/reviewer-run/precheck/14793/head-67a16bfa4003-785579a1 --json core/libs/prx/libSceAgcDriver/tests/execution/Float16MinMaxBits.cpp core/shader/recompiler/Translation/src/FloatInstructions.cpp docs/dev/debt/shader.md |
| negative_controls | not_applicable | No changed Python test module in tools/ can be run as a no-build base-source control. |
Negative control
Not run here. The PR reports a mutation check (swapping min and max fails the test; restoring passes); I did not rerun it.
Findings
Blocking
None.
Non-blocking
- The packed f16 entry and the
v_fma_mixlo_f16entry indocs/dev/debt/shader.mdwere already over 1,000 characters, socheck_conventionswarns on them. The PR keeps that length and says so.
Sources
- Exemption:
AnyPS5/core/shader/recompiler/Translation/src/FloatInstructions.cpp
Lines 13 to 16 in 67a16bf
- Min and max lane path:
AnyPS5/core/shader/recompiler/Translation/src/FloatInstructions.cpp
Lines 61 to 68 in 67a16bf
- Opcode call sites:
AnyPS5/core/shader/recompiler/Translation/src/VectorInstructions.cpp
Lines 952 to 962 in 67a16bf
- Tests: https://github.com/boykopovar/AnyPS5/blob/67a16bfa4003957fe0648197946f492e9f3c66d2/core/libs/prx/libSceAgcDriver/tests/execution/Float16MinMaxBits.cpp
Not verified
- The RDNA 2 ISA citation (§12.10, opcodes 17 and 18) was not re-read; the non-rounding behaviour was checked against the code.
- The GPU runs (NVIDIA RTX 3060 on Windows, lavapipe on Ubuntu) and the mutation check are author-reported.
- Denormal flushing for min and max is unchanged by this PR and was not re-derived.
- The macOS relinker jobs had not finished at review time.
To get this to a good state
- Nothing blocking. Approval waits for the macOS jobs on this head.
✅ NVIDIA GPU test: passedPR head
Intermittent validation messages (1, seen once in the parallel run, not reproduced when the test was re-run alone 2 times; not counted against this PR)Unstable on main (4, 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)Validation IDs already present on main (5 IDs in 484 tests)Skipped (46)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. |
67a16bf to
3533527
Compare
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Validation IDs already present on main (4 IDs in 510 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. |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Tests check implemented or measured behaviour, not that an unimplemented path throws. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3533527 to
8281e58
Compare
What
TranslationContext::packedFloat16(core/shader/recompiler/Translation/src/FloatInstructions.cpp) threwpacked f16 at pc … in FLOAT_MODE … (f16 rounding not to nearest even) is not implementedfor every packed f16 instruction whenever the f16 round field ofFLOAT_MODEis not nearest even. This PR exemptsv_pk_min_f16andv_pk_max_f16from that throw.minMaxF16Bitspath, so no rounding step happens. AMD's RDNA 2 ISA guide, §12.10 (printed p. 164, VOP3P opcodes 17 and 18), defines them as componentwise FP16 minimum and maximum.op_sel, negation and clamp handling is unchanged.v_pk_add/mul/fma/fmac_f16, which do round, still throw.FLOAT_MODE0xcc that thev_fma_mixlo_f16debt entry records for ps5-opengl has the f16 round field set to toward zero, so every packed f16 instruction in such a shader threw before.Debt.
docs/dev/debt/shader.md, two edits:v_fma_mixlo_f16entry's note about packed f16 rounding names the exception.Both entries were already over 1,000 characters on
main, socheck_conventionswarns on them; the PR changes only those clauses and keeps their measurement records.Tested
agc_driver_float16_min_max_bits(tests/execution/Float16MinMaxBits.cpp) gains packed-only runs in every f16 round mode (0 to 3), crossed with:The packed runs use the test's existing
PackedCode, selector and negation checks, and CPU bit model. Rows now include signed zeros, infinities, quiet and signalling NaNs, denormals and minimum normals in every mode. The change passes 696,320 bit-exact comparisons.Mutation check: swapping the min and max selection fails (actual
0x00010000, expected0x00018000); restoring it passes.Required focused comparison:
ctest -R "^agc_driver_float16_min_max_bits$" -Vonmain(f2d874c) and this PR, with the same test source andANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation:packed f16 at pc 20 in FLOAT_MODE 52 (f16 rounding not to nearest even) is not implemented; this PR passes.VK_LOADER_DEBUG=layershowed the validation layer loaded and inserted. Neither log has a missing-layer warning or aVUID-error. This run used 4534907, which differs from this PR only in the debt text.VUID-error.VUID-error.maind2ce337, which changed the add/mul/fma path ofpackedFloat16(perf(shader): drop f16 NaN payload rebuilds that the result never uses #11368), with the same variables on both GPUs:mainfails at the same throw and this PR (3533527) passes with 696,320 comparisons. The layer was inserted, and none of the four logs has a missing-layer warning or aVUID-error.main6f1d2c7, which added a native fma path topackedFloat16(perf(shader): skip impossible NaN repair for normal multipliers #14892), with the same variables on both GPUs:mainfails at the same throw and this PR (8281e58) passes with 696,320 comparisons. The layer was inserted, and none of the four logs has a missing-layer warning or aVUID-error.Wider runs of
-R "agc_driver_.*(f16|float16|half|mode|sdwa|packed)|agc_shader_mode"with the same variables:Neither run has a
VUID-error.python3 tools/check_conventions.py --base origin/main: no errors; twodebt-lengthwarnings on the two entries above, which were already over the limit.python3 tools/pr_overlap.py: no confirmed overlap. The tool could not compare 23 open PRs that conflict withmain, all throughdocs/dev/debt/shader.md. None of their diffs touches the packed-f16 orv_fma_mixlo_f16entries,packedFloat16,v_pk_min/max_f16orFloat16MinMaxBits.cpp. feat(aot): consolidate native ARM64 compiler, runtime and consumers #5021 also editsFloatInstructions.cpp, but only one line invLdexpF16.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