Skip to content

fix(shader): saturate VOP3 v_add/sub_nc_u32 and 16-bit add/sub on clamp - #1898

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
fisherjoey:fix/vop3-add-sub-clamp
Oct 10, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
fisherjoey:fix/vop3-add-sub-clamp

Conversation

@fisherjoey

Copy link
Copy Markdown
Contributor

What

Closes #1897.

This makes the recompiler accept clamp on VOP3 v_add_nc_u32, v_sub_nc_u32, v_subrev_nc_u32, v_add_nc_u16, v_sub_nc_u16, v_add_nc_i16 and v_sub_nc_i16, and saturates the result.

For u32, vAddSubNcU32 already saturated for SDWA, so the three opcodes only needed adding to usesIntegerSaturateClamp.

For the 16-bit ops, a new isVop3Integer16AddSubOpcode lets clamp through checkNativeVop3Modifiers and keeps it in supportsNativeVop3Clamp, and integer16Binary now passes its result through the existing saturateInteger16. The i16 forms now read their operands sign-extended. That doesn't change the low 16 bits when clamp is off.

The semantics were measured with tools/hw-oracle on a Ryzen 5 7600X iGPU (gfx1036, RDNA2, wave32). The settings were IEEE=0, DX10_CLAMP=1, denorm32=0, denorm16=3, round32=0, round16=0, fp16_overflow=0. A second run with IEEE=1, DX10_CLAMP=0, denorm 3/3, round toward zero and fp16_overflow=1 gave the same results. The new test, tests/execution/Vop3AddSubClamp.cpp, pins all 32 measured rows for the 7 clamped ops plus an unclamped v_add_nc_u32 control. The rows include u32 wrap, the u16/i16 boundaries, and inputs with junk in bits 31:16.

The carry ops with clamp still throw. See the issue.

Tested

Nobara Linux 44 (kernel 7.2), Release build.

RADV, Ryzen 5 7600X iGPU (Mesa 26.2.1) NVIDIA RTX 5070 Ti (595.91.07) lavapipe (LLVM 22.1.8)
main + new test fails: ShaderRecompiler::Recompile: VOP3 source modifiers are not implemented fails, same error fails, same error
this branch VOP3 add/sub clamp tests passed passes passes
ctest -R ^agc_ on this branch 250/250 250/250

tools/check_conventions.py --base origin/main passes.

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes

@github-actions github-actions Bot added shader core/shader agc libSceAgc* labels Oct 8, 2026

@klosixgaming-arch klosixgaming-arch left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strong PR. Splitting VAddNcU16/VAddNcI16 (and the sub forms) so the i16 variants sign-extend their operands is a real correctness fix — previously both fell through to the unsigned path. The hardware verification is exemplary for this repo: tools/hw-oracle on gfx1036 with two independent mode settings giving matching results, a 32-row measured table pinned in the test (u32 wrap, u16/i16 boundaries, junk in bits 31:16), and cross-backend runs (RADV / NVIDIA / lavapipe). Reusing saturateInteger16 is clean, and carry-with-clamp still throwing is the correct conservative boundary.

Written with AI assistance.

@johnmaia

johnmaia commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

A real-title data point for this: EVO-PLAYER v0.11.0 (PPSA99039, built from source) compiles its FSR EASU upscaler pixel shader with amdllpc for gfx1013, and that shader starts with two unsigned saturating subtracts:

v_sub_nc_u32_e64 v16, s8, 1 clamp
v_sub_nc_u32_e64 v0, s9, 1 clamp

On main the recompiler rejects them (VOP3 source modifiers are not implemented) when EVO builds its upscale_easu pipeline at start-up. They are the only modifier uses in that shader that main does not accept. With this PR's head 1f0060d7 merged, that pipeline and all 58 of EVO's upscaler pipelines build (80 pipelines in all), and EVO goes on to render and submit its first frame.

Setup: Linux, RX 7600 (RADV, Mesa 25.2.8). The base was main f1451396 with #1916, #1838, #1634, #1714, #1641, #1857, #1529 and #1931 for EVO's earlier stops. Two local patches were also needed to reach this shader, and neither is part of a PR yet: EVO's shader specials corrected to the standard VGT_GS_OUT_PRIM_TYPE/GE_USER_VGPR_EN offsets, and signed compares accepted in the SRT evaluator.

@popcoder565

Copy link
Copy Markdown
Contributor

Native Windows run, since no Windows results were posted. Setup: Windows 10 (19045), RTX 3060 Laptop (NVIDIA 616.92), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release. Head 1f0060d7, merged onto main d70b899 in one build with 33 other AGC/shader PRs that had no Windows run.

No title was run.

🤖 Addressed by Claude Code

@adriadam10

Copy link
Copy Markdown
Collaborator

Checked all 7 clamped forms on the HW oracle (gfx1035): the 32 rows the test pins match bit for bit. Another 525 rows (u16/i16/u32 boundaries crossed with each other, plus random inputs) and op_sel high-half sources match the saturation model. Also checked: the test fails without the change, ctest is green on RADV, and there are no conflicts with main. Merging.

@adriadam10
adriadam10 merged commit 4adefa0 into boykopovar:main Oct 10, 2026
6 checks passed
SleglJan pushed a commit to SleglJan/AnyPS5 that referenced this pull request Oct 10, 2026
…lamp

fix(shader): saturate VOP3 v_add/sub_nc_u32 and 16-bit add/sub on clamp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Recompiler rejects clamp on VOP3 v_add_nc_u32, v_sub_nc_u32 and the 16-bit add/sub ops

5 participants