Repository navigation
fix(shader): translate ds_min/max/add_src2_f32 through dsSrc2 - #14247
Open
StarMrGamer wants to merge 1 commit into
Open
StarMrGamer wants to merge 1 commit into
StarMrGamer wants to merge 1 commit into
Conversation
The f32 src2 forms threw because LDS float atomics did not model the f32 denormal mode. dsAtomic has modelled it since 731575c, so dsSrc2 now sets the same flush fields and the three opcodes use it. The execution test checks each form against ds_min/max/add_f32 under flush and keep modes. Fixes boykopovar#14089 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
MarcZX10
suggested changes
Nov 1, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — changes requested
The implementation and author-reported test evidence are plausible, but the PR overlaps #14085 and does not record an agreed landing order as the repository requires.
What I ran
Reviewed head 0f8ec048111e27629b48385e46b4e98cf6b3eca3 against PR base 5d3ad2112f60f35d463dc2c9d96d1c9d35871015. The fresh merge preview against current main 4a58bc29ad656ad7a54c1733b3b683e68b7ccf85 was clean. Environment: Linux x86_64, Python 3.11.2, GCC 12.2.0. No build or compile was run.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14247
python3 tools/pr_stack_check.py 14247 --repo boykopovar/AnyPS5
python3 tools/pr_abi_audit.py --repo boykopovar/AnyPS5 --base 5d3ad2112f60f35d463dc2c9d96d1c9d35871015 --head 0f8ec048111e27629b48385e46b4e98cf6b3eca3 --json
python3 tools/pr_test_quality.py --root /marc/AnyPS5 --json core/libs/prx/libSceAgcDriver/tests/execution/LdsSrc2.cpp core/shader/recompiler/Translation/src/MemoryInstructions/MemoryInstructionDispatch.cpp core/shader/recompiler/Translation/src/MemoryInstructions/SharedMemoryInstructions.cpp docs/dev/debt/shader.mdResults
| Check | Result |
|---|---|
git diff --check and conventions/template |
Passed in precheck. |
| Current-main merge preview | Clean against 4a58bc29ad656ad7a54c1733b3b683e68b7ccf85; no declared dependency found. |
| ABI audit | Could not run: the helper could not resolve this remote repository checkout from its --repo argument. No ABI change is claimed. |
| Test-quality heuristic | Warned that LdsSrc2.cpp may be unregistered. This is a false positive: the execution tests are included through CMake's tests/execution/*.cpp glob and registered by agc_add_default_test. |
| Hosted CI at refresh | eligibility, intake, report, and library-portability passed; conventions, labels, and the relinker/Windows build jobs were queued or in progress; Linux, register-context, and write-watch jobs were skipped. These statuses are hosted CI, not local runs. |
| Author-reported tests | The PR reports paired focused Vulkan tests passing 3/3 on baseline and change, plus 165/165 LDS/shader tests (2 self-skips). These were not independently run. |
| Local build/tests | Not run; the C++/Vulkan tests require built binaries and GPU execution. |
Raw output excerpts
Precheck:
All conventions checks passed.
Head unchanged at completion: True
Base unchanged at completion: True
Fresh stack check:
Target branch tip: `4a58bc29ad656ad7a54c1733b3b683e68b7ccf85`
All heads unchanged: **True**
- PR #14247: **clean** against current base; head `0f8ec048111e27629b48385e46b4e98cf6b3eca3`; changed paths: none reported
No declared PR dependencies were found in the target PR chain.
ABI audit:
ABI audit unavailable: fatal: cannot change to '/marc/AnyPS5/boykopovar/AnyPS5': No such file or directory
Test-quality heuristic:
{
"findings": [
{
"path": "core/libs/prx/libSceAgcDriver/tests/execution/LdsSrc2.cpp",
"line": 1,
"kind": "possible-unregistered-cpp-test",
"message": "Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage."
}
],
"negative_control_candidates": [],
"limits": [
"Heuristics can miss weak assertions and can flag valid tests; inspect every finding.",
"A test that passes against base is not by itself proof that the PR test is inadequate.",
"Negative controls are only candidates until the test actually runs against base and fails by assertion rather than import/setup error."
]
}
Negative control
| Assertion | Change in control | Result |
|---|---|---|
| The src2 float atomics follow the denormal-mode behavior of their existing ds counterparts | The author reports removing the two new denormal-flush assignments | The focused test reportedly failed with lds src2: min differs from ds_min_f32 in mode 0x00000000; not independently run. |
Findings
Blocking: The PR changes the same DS src2 debt entry as open #14085. Its description says whichever lands second will rebase, but does not identify an agreed order, and its checklist says Depends on: none. The repository contribution rule asks authors of overlapping PRs to agree which lands first and record that dependency. Please coordinate with #14085's author and record the agreed order in the checklist.
Non-blocking: No implementation or test-source defect was found in the inspected changes.
Sources
- Overlap/dependency rule at the PR base
- #14247 debt-entry edit
- #14085 overlapping debt-entry edit
- Execution-test registration at the reviewed head
Not verified
No local build, CTest, or GPU validation run was performed. The reported test runs are author-provided, not independently reproduced. Hosted CI was still in progress at the latest status refresh.
To get this to a good state
Coordinate with #14085's author and record which PR lands first. I will recheck the overlap and repeat applicable checks on the updated head.
Contributor
Author
|
Thanks for the review. I've asked on #14085 whether its author will leave the DS src2 bullet to this PR, the way #14085 left the AI-assisted: yes (Claude Code). |
12 tasks done
|
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.
|
Contributor
✅ NVIDIA 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 (3, not counted against this PR)Validation IDs already present on main (5 IDs in 480 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. |
LeandroLP23
added a commit
to LeandroLP23/AnyPS5
that referenced
this pull request
Nov 1, 2026
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
ds_min_src2_f32,ds_max_src2_f32andds_add_src2_f32threw at translation because LDS float atomics did not model the f32 denormal mode. Since 731575cdsAtomicsetsflushDenormalsandflushDenormalResultsfromFLOAT_MODEbits 4 and 5.dsSrc2now sets the same two fields on theDS[A]atomic, and the three opcodes go through it withSharedAtomicFMin32,SharedAtomicFMax32andSharedAtomicFAdd32. The 64-bit, GDS andoffset1[7]forms still throw.Source of the semantics: the f32 src2 forms are taken to be the measured
ds_min_f32,ds_max_f32andds_add_f32(RDNA2, gfx1035, the oracle's hardware) applied toDS[A]andDS[B], which is how the integer src2 forms were done. No RDNA2 part has the src2 opcodes, so they are not measured on GFX10.1; the DS src2 entry indocs/dev/debt/shader.mdnow says so instead of giving the obsolete reason. This is the inference the question in #14089 asks about.agc_driver_lds_src2replaces the check that the f32 forms are refused with an execution check: for each op it stores the denormals0x1and0x2in LDS, runs the src2 form and the non-src2 form on them underFLOAT_MODE0x00 and 0x30, and requires that both forms give the same result in each mode and that the two modes differ (0 in both forms under 0x00; 1, 2 and 3 under 0x30). With the twodsSrc2flush lines removed, it fails withlds src2: min differs from ds_min_f32 in mode 0x00000000.#14085 rewords the same debt sentence; whichever lands second rebases.
Tested
Arch Linux, AMD Radeon 780M (RADV PHOENIX, Mesa 26.2.5), GCC 16.2.1, Release.
ctest --test-dir build -R 'agc_driver_lds_src2|agc_driver_lds_float_atomic_modes|agc_driver_lds_add_f32' -VwithVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1, onmaindc9dc7e and on this branch on top of it: 3/3 passed on both, neither log containswas not found but was requested by env var VK_INSTANCE_LAYERS, and neither has anyVUID-line.ctest -R 'lds|shader|recompiler|ds_'withANYPS5_REQUIRE_VULKAN=1: 165/165 passed (2 skipped by the tests themselves).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