Skip to content

feat(shader): model ds_ordered_count swap - #7429

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
ps54all:feat/shader-ds-ordered-count-swap
Oct 19, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
ps54all:feat/shader-ds-ordered-count-swap

Conversation

@ps54all

@ps54all ps54all commented Oct 18, 2026

Copy link
Copy Markdown

What

ds_ordered_count with offset1 bit 4 set (swap instead of add, as LLVM encodes llvm.amdgcn.ds.ordered.swap) throws ordered swap is not modeled. God of War Ragnarök (PPSA08330) dispatches a compute shader (0x10527b8200) that uses it, and stops there:

[gpu] driver failure: compute shader 0x10527b8200: shader 0x10527b8200 stage 0: ShaderRecompiler::Recompile: ds_ordered_count at pc 16504: ordered swap is not modeled

The swap now goes through the same path as the ordered add (AppendConsume with ordered): a new IR opcode DataOrderedSwap with the operands of DataOrderedCount, emitted as one device-scope OpAtomicExchange of the first active lane's value on the GDS counter, with the counter's previous value returned to every active lane. As for the ordered add, the waves are not serialized in launch order, so which wave's value the counter keeps, and which previous value each wave gets, need not match the hardware; the technical debt entry for ds_ordered_count says so. Counts of more than one dword and LDS forms still throw.

Tested

Checklist

  • Based on current main; no other open PR implements the same functions (fix(shader): add the ordered count of a first active lane in the upper wave64 half #6258 changes the ordered add's lane, not the swap)
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw; no silent stubs added
  • No notes, investigation .md files or images added to the repository
  • New third-party code is a submodule built from source (none added)
  • Generic behaviour, not specific to one title
  • Depends on: none (the Ragnarök result above also needs feat(agc): link calls through the AGC FS table #6160)
  • Unblocks: God of War Ragnarök (PPSA08330) past this compute shader
  • GPUs tested: NVIDIA GeForce RTX 3080, driver 610.74, Windows 11
  • AI-assisted: yes (Claude Code). I am responsible for the change.

🤖 Generated with Claude Code

@github-actions

github-actions Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 4072 core/shader/recompiler/IntermediateRepresentation/include/IntermediateRepresentation/IrOpcode.hpp
core/shader/recompiler/IntermediateRepresentation/src/IrOpcode.cpp
core/shader/recompiler/SpirvBackend/include/SpirvBackend/SpirvEmitterInstructions.hpp
core/shader/recompiler/SpirvBackend/src/SpirvFlowEmitter.cpp
PR 5021 core/shader/recompiler/SpirvBackend/src/SpirvFlowEmitter.cpp
core/shader/recompiler/SpirvBackend/src/SpirvMemory/SpirvMemoryInstructions.cpp
docs/dev/debt/shader.md
PR 5371 docs/dev/debt/shader.md
PR 5426 docs/dev/debt/shader.md
PR 5500 docs/dev/debt/shader.md
PR 5941 docs/dev/debt/shader.md
PR 6009 docs/dev/debt/shader.md
PR 6235 docs/dev/debt/shader.md
PR 6280 docs/dev/debt/shader.md
PR 6412 docs/dev/debt/shader.md
PR 6488 docs/dev/debt/shader.md
PR 6604 docs/dev/debt/shader.md
PR 6924 docs/dev/debt/shader.md
PR 6947 core/shader/recompiler/SpirvBackend/src/SpirvFlowEmitter.cpp
PR 6977 docs/dev/debt/shader.md
PR 7223 docs/dev/debt/shader.md
PR 7332 core/shader/recompiler/SpirvBackend/src/SpirvFlowEmitter.cpp
core/shader/recompiler/SpirvBackend/src/SpirvMemory/SpirvMemoryInstructions.cpp
docs/dev/debt/shader.md
PR 7392 core/shader/recompiler/SpirvBackend/src/SpirvFlowEmitter.cpp
docs/dev/debt/shader.md
PR 7395 docs/dev/debt/shader.md
PR 7484 docs/dev/debt/shader.md
PR 7642 core/shader/recompiler/SpirvBackend/src/SpirvFlowEmitter.cpp
PR 7684 docs/dev/debt/shader.md

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

ps54all commented Oct 18, 2026

Copy link
Copy Markdown
Author

On the overlap with #6258: both change AppendConsume, but they don't implement the same thing. #6258 fixes which lane the ordered amount is read from (a wave64 wave whose first active lane is in the upper half); this PR adds the swap, an OpAtomicExchange of that same amount. So the swap picks up #6258's fix as is once both are in.

A trial merge of the two touches no source conflict; only the tests conflict:

Whichever merges second can rebase those two files. Neither depends on the other.

AI-assisted: yes

@wachi35676

Copy link
Copy Markdown
Contributor

Tested at edd5059ee on Windows 11 (GCC 15.2.0, Release), merged onto main 9f25d0bf5, on an RTX 5070 Ti (617.42) and an AMD Radeon iGPU, RDNA2 (1002:13c0, 32.0.21042.62, ANYPS5_GPU=AMD).

  • In ctest -R "agc_|shader|recompiler|scalar_call" (715 tests), no test changes status against main on either GPU.
  • agc_driver_gds_append_consume and agc_wave_control_decode pass on both. With the recompiler sources set back to main, they fail with ds_ordered_count at pc 16: ordered swap is not modeled.

One case the test doesn't reach: a wave64 whose first active lane is in the upper half. My local probe ran 64 threads with s_mov_b32 exec_lo, 0 before the ordered op and lane value tid+3, so the expected amount is 35.

  • RTX 5070 Ti (32-wide subgroups): swap leaves counter 0 at 3 instead of 35, and ordered add gives 1003 instead of 1035. main's add also gives 1003: amount is shuffled from first & 31 in the low half.
  • AMD (64-wide subgroups): 35 and 1035, which is correct.

With #6258's AppendConsume hunk on top of this PR, NVIDIA also gives 35 and 1035. Swap and add share amount, so this PR and #6258 should land together; they conflict only in the tests.

For the semantics: LLVM SIISelLowering.cpp at llvmorg-19.1.0 (lines 8844-8894) sets offset1 bit 4 for ds.ordered.swap. The RDNA2 ISA guide (DS_ORDERED_COUNT, page 206) documents only the add, with count = src0[first valid lane].

AI-assisted: yes (Claude Code); results above were run on this machine.

@blawesom

Copy link
Copy Markdown
Contributor

Rebase handoff, with one test-adaptation caveat — rebased onto current main (authorship unchanged). The test conflicts resolved: main's wave64/subgroup-refusal coverage kept alongside your swap coverage, the duplicate OrderedM0Literal deduplicated, and your swap loop given its own OrderedM0 iteration.

The caveat: agc_driver_gds_append_consume fails on the merged head (wave 0 returned 1003, expected 1000 — the ordered-add assertion with M0 0 from lane 0). Your own unrebased head passes, so the failure comes from my adaptation: main's CheckOrdered takes a firstLane parameter your version lacked, and I passed workgroup.waveSize for the swap loop's runs — the wrong value, evidently. The swap-loop call needs the first-lane semantics your original used. The feature itself (the swap path) is untested by that wrong adaptation, so treat the branch as rebase-complete but test-pending on that one call.

Everything else is green apart from the four known main regressions (#5444 Mesa, #7584 BDA, #7766, #7784); conventions pass.

To pick it up:

git fetch https://github.com/blawesom/AnyPS5 rebase/7429
git checkout <your branch>
git merge --ff-only FETCH_HEAD

Head is b13be82e2.

@adriadam10 adriadam10 self-assigned this Oct 19, 2026
A ds_ordered_count with offset1 bit 4 set swaps the first active lane's value into the GDS counter instead of adding it. Emit it as one device-scope atomic exchange through the same path as the ordered add, and return the counter's previous value to every active lane.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@adriadam10
adriadam10 force-pushed the feat/shader-ds-ordered-count-swap branch from edd5059 to b0558ce Compare October 19, 2026 14:51
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 19, 2026
@DavidAmsellem

Copy link
Copy Markdown
Contributor

First AMD-on-Linux (RADV) run of this PR. Merged onto main dbac9fa15 (clean merge), built in full and compared with the same main, on the same machine.

AMD Radeon RX 7900 XT (RADV NAVI31, gfx1100), Mesa 25.2.8, Linux Mint 22.3 (Ubuntu 24.04 base), kernel 7.0.0-38, GCC 13.3, Release.

Full suite (ANYPS5_REQUIRE_VULKAN=1 ctest -j8): 1638/1640 with this PR, against 1638/1640 on main. The failures are the same two on both, so this PR adds none: agc_driver_image_table_vouched_lookups (#7766, fixed by #7916) and agc_driver_guest_memory_lifetime (#7784).

The tests around the change all run and pass: agc_wave_control_decode, agc_driver_gds_append_consume, agc_driver_ds_append_consume and agc_atomic_gds_size. The PR adds no new ctest entry, and no test is skipped or removed compared with main.

GPUs tested: AMD Radeon RX 7900 XT (RADV NAVI31, gfx1100), Mesa 25.2.8, Linux Mint 22.3 / kernel 7.0.0-38

AI-assisted: yes

@DavidAmsellem

Copy link
Copy Markdown
Contributor

First AMD-on-Linux (RADV) run of this PR. Merged onto main dbac9fa15 (clean merge), built in full and compared with the same main, on the same machine.

AMD Radeon RX 7900 XT (RADV NAVI31, gfx1100), Mesa 25.2.8, Linux Mint 22.3 (Ubuntu 24.04 base), kernel 7.0.0-38, GCC 13.3, Release.

Full suite (ANYPS5_REQUIRE_VULKAN=1 ctest -j8) at 0289b9930: 1638/1640 with this PR, against 1638/1640 on main. The two failures are the same on both, so this PR adds none: agc_driver_image_table_vouched_lookups (#7766, fixed by #7916) and agc_driver_guest_memory_lifetime (#7784).

Comparing the JUnit output with main: this PR adds no ctest entry, and none is skipped or removed.

This checks correctness only; I did not measure performance.

GPUs tested: AMD Radeon RX 7900 XT (RADV NAVI31, gfx1100), Mesa 25.2.8, Linux Mint 22.3 / kernel 7.0.0-38

AI-assisted: yes

@blawesom

Copy link
Copy Markdown
Contributor

Follow-up on my rebase caveat, now resolved by your head b0558ce95 — rebased cleanly onto current main and the agc_driver_gds_append_consume tests pass (my earlier wrong CheckOrdered firstLane adaptation is gone with your rewrite). Full suite green apart from the four known main regressions (#5444 Mesa, #7584 BDA, #7766, #7784). Conventions pass (one pre-existing debt-length warning in shader.md, not from this PR). The handoff branch blawesom:rebase/7429 is updated to this head if you want the pre-verified rebase. Also worth noting @DavidAmsellem's independent RADV run above (1638/1640, same two known failures) matches this.

@adriadam10
adriadam10 merged commit 602d8e6 into boykopovar:main Oct 19, 2026
10 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Rebased onto current main; the only conflicts were in the tests, since main added the first-lane/wave64 runs to CheckOrdered.

What I changed on top of your commit:

  • GdsAppendConsume.cpp: the swap now runs for every OrderedRun (M0 and first lane, including the upper wave64 half) instead of its own M0 loop. CheckOrderedSwap takes the first lane, checks that lanes below it stay untouched, and compares the returned values plus the final counter with the start value and each wave's first active lane value.
  • WaveControlDecode.cpp: swap moved from the refused list to the accepted ones next to the wave64-on-32-lane check.

Checked: the swap shares amount with the ordered add, DataOrderedSwap is in LaneSource and SharedAccessOf, the TechnicalDebt entry describes the unordered waves, and the swap/count/LDS limits still throw. Not measured on hardware: which lane's value the swap stores (the ISA guide documents only the add). Thanks @wachi35676 for the NVIDIA/AMD runs; merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* docs shader core/shader unblocks title Names a title ID on the Unblocks line

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants