Skip to content

fix(shader): guard runtime MODE changes - #1854

Merged
adriadam10 merged 2 commits into
boykopovar:mainfrom
Sanjaysrinivas:fix/shader-mode-registers
Oct 10, 2026
Merged

adriadam10 merged 2 commits into
boykopovar:mainfrom
Sanjaysrinivas:fix/shader-mode-registers

Conversation

@Sanjaysrinivas

@Sanjaysrinivas Sanjaysrinivas commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

What

Part of #1668's shader-mode workstream and #981. MODE rounding reads currently return zero even when the shader declares a nonzero initial mode, and MODE writes can silently leave that declared state inconsistent with the shader's instructions.

For example, with FLOAT_MODE=0xc5, reading MODE bits 0..3 now returns 5. An immediate write of 5 preserves the state and is accepted; writing 0 reports the instruction and program counter as an unsupported runtime transition.

  • Read any subfield confined to MODE bits 0..3 from the initial mode already carried by GuestContext::floatMode.
  • Accept s_setreg_imm32_b32 only when its selected rounding bits remain unchanged, masking source bits above the selected width. Apply the same rule to the low four bits of s_round_mode; its upper immediate bits are ignored.
  • Reject SGPR-sourced MODE writes and state-changing immediate writes. Runtime transitions need control-flow-aware state and matching arithmetic lowering.
  • Document the data flow, supported contract and remaining gaps in Architecture and TechnicalDebt.

Semantics follow AMD's RDNA2 ISA sections 5.8 and 6.4, Table 24, cross-checked against LLVM 20.1.8's AMDGPUBaseInfo.h, SIModeRegister.cpp and GFX10 assembler tests. Exact source links are in Architecture. No console or GPU execution measurements are claimed. Missing metadata retains the existing nearest-even assumption; other MODE fields remain unsupported, and non-MODE s_setreg_b32 writes retain their existing no-op behavior, recorded in TechnicalDebt.

This is partial progress on #1668; it does not complete the category issue or add arithmetic support for every initial mode. Initial-mode propagation (#1816), output-modifier mode handling (#1851) and f32 denormal flushing (#1850) are already on main. No open PR prerequisite.

Coordination with #1811: both PRs initially edited the same CMake test-list line. The MODE test is now registered separately, with its normal-build inclusion, Windows runtime path and timeout preserved. Combining the two changes merges cleanly in either order; neither PR depends on the other. The MODE documentation also sits within the existing Graphics section so it can merge independently of #1802's appended kernel documentation.

Tested

Release builds on Windows 11 Pro (build 26300, WinLibs GCC 15.2.0 r7) and Ubuntu 24.04 under WSL2 (GCC 13.3.0). Four focused CTest cases passed on each host, with no skips: agc_shader_mode_register, agc_shader_control_flow, agc_shader_disk_cache, and agc_validate_program.

The new regression uses production decoding, control-flow construction and translation, with requirements active in Release:

  • 5,120 known-mode subfield read comparisons across all 256 FLOAT_MODE bytes and wave32/wave64, plus legacy unknown-metadata reads.
  • 1,024 masked immediate-write combinations and 512 s_round_mode combinations, including ignored upper bits and preservation of untouched rounding bits.
  • SGPR writes, unsupported selectors, branch-local writes with diagnostics at PC 4, and initial-mode propagation through full Recompile.
  • A 20-second CTest timeout; the new test took about 0.13 seconds on Linux and 0.99 seconds on Windows.

LLVM 18.1.3's assembler independently verified 23 instruction encodings for both gfx1013 and gfx1030. Replacing the production fix with upstream code makes the final regression fail; restoring it returns all four Linux cases to passing.

Local builds used an untracked CMake parent importing the production libc/recompiler source lists and the four production test target definitions, with SPIRV-Tools disabled. After rebasing onto main ce64d8eb, all four cases passed again on each host; the parent also evaluates the production MODE test registration and checks its inclusion in normal builds. This covers compiler regressions, not the full application build, GPU execution, a title or console behavior. Full repository CI passed on Linux and Windows at the earlier head 4344bbfd. After the rebase onto main 8b393b6e9a8ade9188d4f74a0293e0efd9828597, the four cases pass again on Ubuntu 24.04/WSL2 (GCC 13.3.0, Release, 4/4, normal repository build); the rebased head 3df8f69e7bdebaa8903f834d604fcc9d75f2f831 needs a fresh CI run.

cmake --build <local-build> --target agc_shader_mode_register_tests agc_shader_control_flow_tests agc_shader_disk_cache_tests agc_validate_program_tests --parallel 6
ctest --test-dir <local-build> --output-on-failure --parallel 4 -R '^agc_(shader_(mode_register|control_flow|disk_cache)|validate_program)$'
python tools/check_conventions.py --base origin/main --title 'fix(shader): guard runtime MODE changes' --body <pr-body-file>

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; existing non-MODE register-write gaps are listed in TechnicalDebt
  • No notes, investigation .md files or images added to the repository
  • New third-party code is a submodule built from source (none added)
  • Generic behavior, not specific to one title
  • Depends on: none
  • AI-assisted: yes

@github-actions github-actions Bot added build shader core/shader docs agc libSceAgc* labels Oct 8, 2026
@Sanjaysrinivas
Sanjaysrinivas force-pushed the fix/shader-mode-registers branch 2 times, most recently from 0805875 to 18ed85f Compare October 8, 2026 22:32
@Sanjaysrinivas
Sanjaysrinivas marked this pull request as ready for review October 8, 2026 22:53

@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.

This is the bar for shader-semantics PRs in this repo: RDNA2 ISA §5.8/§6.4 Table 24 plus a LLVM 20.1.8 cross-reference with exact source links, a 5,120-comparison regression (all 256 FLOAT_MODE bytes, wave32/wave64), independent LLVM-assembler encoding verification, and the remaining gaps explicitly recorded in TechnicalDebt rather than silently stubbed. Runtime MODE transitions correctly throw with instruction + PC, and the ARCHITECTURE.md addition is clear about what is and isn't modeled. No console/GPU measurements are claimed where they don't exist — exactly right.

Written with AI assistance.

@CouchRiv

CouchRiv commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Measured on RDNA2 (gfx1036, Radeon 610M) with tools/hw-oracle, wave32. The kernel's float mode is set through the descriptor and read with s_getreg_b32 hwreg(HW_REG_MODE, ...):

float mode MODE MODE[3:0]
0xc0, IEEE=0, DX10_CLAMP=1 0x1c0 0x0
0xc5, IEEE=0, DX10_CLAMP=1 0x1c5 0x5
0xfb, IEEE=1, DX10_CLAMP=0 0x2fb 0xb

Writes, starting from 0xc5: s_round_mode 0x1 reads back 0x1c1, s_setreg_imm32_b32 hwreg(HW_REG_MODE, 0, 2), 0xfffffff0 writes 0 (bits above the width dropped), and s_round_mode 0xf3 sets only the four round bits (0x1c3). Each write changes the rounding of the very next v_add_f32, with no wait in between. This matches the read value and the masking rules in the PR. On main, the same MODE[3:0], [1:0] and [3:2] reads return 0 0 0 where hardware gives 5 1 1 (0xc5) and 0xb 3 2 (0xfb), on RTX 4070 Ti and RADV.

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 9, 2026
@Sanjaysrinivas
Sanjaysrinivas force-pushed the fix/shader-mode-registers branch from 18ed85f to 3df8f69 Compare October 9, 2026 20:59
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 9, 2026
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

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.

Pull request Conflicting files (lines on main) Same exports Same new files
#1190 docs/dev/TechnicalDebt.md L307-308

Sanjaysrinivas and others added 2 commits October 10, 2026 03:16
Read MODE rounding fields from the declared initial mode. Accept only
constant writes that preserve it; diagnose unsupported runtime changes.
Document the supported contract and add decoder-to-recompiler regressions.

Refs boykopovar#1668, boykopovar#981
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked on the HW oracle (gfx1035, wave32) across 32 initial modes (all 16 rounding combinations × two denorm/IEEE/DX10 settings): the ten MODE[3:0] subfield reads, s_setreg_imm32_b32 on (0,4), (0,2), (2,2), (1,2) with all 16 low values and upper bits set, and s_round_mode 0..15 with and without bit 15. 3392 checks, 0 mismatches with the PR: reads return the initial bits, writes are masked to the width, and the PR accepts exactly the writes that leave MODE unchanged. Matches @CouchRiv's gfx1036 run. Build + ctest of libSceAgc/libSceAgcDriver green, and agc_shader_mode_register fails without the fix.

Added on your branch:

  • Rebased onto current main.
  • Moved the new "Shader MODE register access" section in docs/dev/ARCHITECTURE.md below the Recompiler.cpp / ShaderDiskCache bullets, which describe the diagram above it.

Merging once CI is green.

@adriadam10
adriadam10 force-pushed the fix/shader-mode-registers branch from 3df8f69 to 4120b2b Compare October 10, 2026 06:04
@adriadam10
adriadam10 merged commit 692c397 into boykopovar:main Oct 10, 2026
14 checks passed
SleglJan pushed a commit to SleglJan/AnyPS5 that referenced this pull request Oct 10, 2026
…e-registers

fix(shader): guard runtime MODE changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* build docs shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants