Skip to content

fix(agc): refuse LDS split before allocation - #7827

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
MarcZX10:codex/fix-lds-split-before-allocation-7794
Oct 21, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
MarcZX10:codex/fix-lds-split-before-allocation-7794

Conversation

@MarcZX10

@MarcZX10 MarcZX10 commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

What

Move the unsupported LDS split check to the start of recordDispatch, before dispatch recording allocates scratch or submits GPU work. Preserve main's CPU-resolved-indirect diagnostic context. The regression requires the LDS-specific refusal message and checks both an ordinary oversized Y dispatch and a z = UINT32_MAX case that distinguishes the early guard from the later refusal.

Fixes #7794.

Tested

  • Rebased head 82be87f185: git diff --check upstream/main...HEAD passed.
  • Rebased head 82be87f185: python3 tools/check_conventions.py --base upstream/main --head HEAD passed.
  • Earlier head 6a189b3dfc: configured and built agc_driver_oversized_dispatch_tests on Linux x86-64. CTest could not execute because this container's Intel Xeon E5-1620 v2 lacks AVX2, FMA, BMI1/BMI2, LZCNT and MOVBE; AnyPS5 aborts during host CPU validation before Vulkan setup.
  • A collaborator tested a stronger composition of this change on main 7a36de66b9: NVIDIA RTX 5080 and AMD Radeon Graphics showed 3/3 passes each, with a negative control distinguishing the early refusal. That hardware report predates current main 43a2abaf77; this rebased head has not been rerun on those GPUs. See the detailed report in the PR discussion.
  • No local GPU test was run on the rebased head.

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
  • No notes or investigation files added
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none (#6786 is in current main)
  • Fixes: AGC: an unsupported LDS dispatch split allocates scratch before refusing it #7794
  • Unblocks: none
  • GPUs tested: see earlier composition report above; not retested on current main
  • AI-assisted: yes

@MarcZX10
MarcZX10 force-pushed the codex/fix-lds-split-before-allocation-7794 branch from 29d3972 to 6a189b3 Compare October 19, 2026 09:13
@MarcZX10 MarcZX10 changed the title Fix LDS split refusal ordering fix(agc): refuse LDS split before allocation Oct 19, 2026
@github-actions

github-actions Bot commented Oct 19, 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 1395 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 2434 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 4040 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 4072 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 4472 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 5021 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 5431 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 5441 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 5499 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 5925 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 6188 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 6202 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 6290 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 6788 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 6947 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 6960 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 7201 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 7275 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 7449 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 7484 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 7642 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 8014 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 8105 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 8278 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp
PR 8847 core/libs/prx/libSceAgcDriver/Execution/src/VulkanDevice.cpp

@github-actions github-actions Bot added agc libSceAgc* fix error Closes an issue (Fixes #N) labels Oct 19, 2026
@Neithennn

Copy link
Copy Markdown
Contributor

Windows NVIDIA run, since the host CPU guard stopped the test on your machine.

Windows 11 Pro 26200, NVIDIA GeForce RTX 4080 (driver 617.42), AMD Ryzen 7 5800X, WinLibs GCC 15.2.0 r7, Ninja, Release, bundled dependencies, BUILD_TESTING=ON, ANYPS5_NO_SHADER_CACHE=1, ANYPS5_REQUIRE_VULKAN=1. Head 6a189b3df merged onto main 36f60df9e; full build succeeds.

  • agc_driver_oversized_dispatch: passes (targeted, full suite and 5 serial runs); it prints oversized dispatch tests passed.
  • The other LDS tests (agc_driver_lds_*, agc_driver_pixel_lds*, agc_lds_lock_budget, agc_spirv_lds_atomic_lock, agc_mubuf_lds_decode): 20/20 pass.
  • Full ctest -j 8: 1631/1631. Unchanged main on the same machine: 1631/1631.

Negative control: with only Execution/src/VulkanDevice.cpp reverted to main (the tightened OversizedDispatch.cpp kept), agc_driver_oversized_dispatch still passes on this GPU. So on the RTX 4080 the refusal is already reached without the reordering, and the test does not tell the two versions apart here. I did not look into which earlier failure the reordering avoids on other devices.

AI-assisted: yes (runs done with Claude Code).

@LeandroLP23

Copy link
Copy Markdown
Contributor

Ran agc_driver_oversized_dispatch on AMD and Windows. AMD Radeon 860M (RDNA 3.5, 1002:1114), driver 32.0.13058.8001, Windows 11, WinLibs GCC 15.2.0, Release, ANYPS5_NO_SHADER_CACHE=1. Head merged into main 118266a (clean merge). "Main's sources" means this PR's test files over main's other files.

It passes 3 of 3, but on this GPU it can't exercise the change. The split case skips:

oversized dispatch X split: skipped, the device reports a 4294967295 workgroup limit, which never splits a 129199x1x1 dispatch

So it also passes with main's sources. AMD's Windows driver reports a 2^32-1 workgroup count limit, so no dispatch is ever split there. The LDS refusal path needs a GPU with the 65535 limit, such as NVIDIA.

AI-assisted: yes (Claude Code)

@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 19, 2026
@Amoenus

Amoenus commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

@MarcZX10 #6786 has merged. I validated a rebased composition 6932eafa20 of your 6a189b3dfc on main 7a36de66b9, retaining your authorship and main's cpuArguments / cpuReason annotations in the relocated LDS diagnostic. It also retains the stronger test from the existing #7794 fork proposal: the original Y case plus z=UINT32_MAX, both requiring the intended LDS refusal.

Three fresh processes on each physical GPU Result
Strict fixture only, main production unchanged 3/3 expected wrong-refusal failures on RTX 5080 and 3/3 on Radeon Graphics
Rebased early refusal + same fixture 3/3 passes on each GPU, no VUID/errors/warnings in oversized dispatch

The larger-Z control distinguishes the versions even when the first case reaches main's late LDS check. Physical X splitting is unavailable at the reported limits; Y splitting and both LDS refusals execute. This addresses the earlier nondiscriminating-control concern without claiming X coverage.

Full default and explicit relinker/library builds pass. Oversized dispatch, valid device-memory LDS, cross-workgroup flag, indirect limit and pending-producer checks run three times per GPU, plus CPU graphics/PM4 three times: 36 behavioral passes (30 GPU, 6 CPU). The 30 common main checks pass too. The pending-producer fixture has 12 validation errors on main and on this composition, isolated in new #7868; no additional VUIDs, warnings, whole-test skips or timeouts. This is not a blanket validation-clean result.

Windows 11 Enterprise 26200 / GCC 15.2 r7; NVIDIA RTX 5080 (10de:2c02), driver 610.62 / 32.0.16.1062; AMD Radeon(TM) Graphics (1002:13c0), driver 32.0.21036.18. Required Vulkan, disabled application shader cache, Khronos 1.4.363 confirmed loaded, verbose serial CTest, 30 s per process. New main bbb45463c7 landed after the freeze and is not covered by these results. Linux, the unchanged old PR head and title impact were not retested.

Cppcheck/clang-tidy ran too: the fixture has no selected findings; 10 untriaged findings point to unedited device-initialization/header locations, so no analyzer-clean claim. The tested composition is available on our fork for your rebase. #7794 updated in place; I am supporting this PR rather than submitting a duplicate.

AI-assisted adaptation, validation and report: yes, Codex.

@MarcZX10
MarcZX10 force-pushed the codex/fix-lds-split-before-allocation-7794 branch from 6a189b3 to 82be87f Compare October 19, 2026 15:17
@MarcZX10

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (43a2abaf77), preserving main’s CPU-resolved-indirect diagnostic context while moving the LDS refusal ahead of later dispatch work. I also added the z = UINT32_MAX regression case from the earlier tested composition report; the current head has not been rerun on those GPUs. Current git diff --check and conventions pass. The earlier CMake build was on the pre-rebase head, and local runtime execution remains blocked by this host CPU’s missing AVX2/BMI features.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 19, 2026
@LeandroLP23

Copy link
Copy Markdown
Contributor

Reran the rebased head 82be87f on AMD's Windows driver. The new z = UINT32_MAX case tells the two versions apart on this GPU, which the earlier fixture couldn't. Radeon 860M (RDNA 3.5, 1002:1114), driver 32.0.13058.8001, Windows 11, WinLibs GCC 15.2.0, Release, ANYPS5_NO_SHADER_CACHE=1. Head merged into main 88a6b34 (clean merge).

agc_driver_oversized_dispatch Result
This head pass, 3 of 3
main's sources, this PR's test fails: oversized dispatch: wrong LDS split refusal: Vulkan dispatch: the LDS of 1x65536x4294967295 workgroups of 36872 bytes each is over maxStorageBufferRange 4294967295

The X split case still skips here (the device reports a 4294967295 workgroup limit, which never splits a 129199x1x1 dispatch), as before. The Z case doesn't depend on that limit.

AI-assisted: yes (Claude Code)

@MarcZX10

Copy link
Copy Markdown
Contributor Author

Thanks for rerunning the rebased head with the z = UINT32_MAX case. This provides a distinguishing negative control on the Radeon 860M: the PR passes 3/3 and main’s sources fail with the expected oversized-LDS refusal. The X-split case remains unexercised on this device because its reported workgroup limit never triggers that split; I’ll keep that limitation clear.

@adriadam10 adriadam10 self-assigned this Oct 19, 2026
@Neithennn

Copy link
Copy Markdown
Contributor

RTX 4080 / Windows run of the rebased head 82be87f18: the new z = UINT32_MAX case tells the two versions apart on NVIDIA too.

Windows 11 Pro 26200, NVIDIA GeForce RTX 4080 (driver 617.42), WinLibs GCC 15.2.0 r7, Ninja, Release, BUILD_TESTING=ON, ANYPS5_NO_SHADER_CACHE=1, ANYPS5_REQUIRE_VULKAN=1. Head merged onto main 27bcd6a29.

Tree agc_driver_oversized_dispatch
this head passes 5/5 serial runs
this head with Execution/src/VulkanDevice.cpp from main (the PR's test kept) fails 5/5: oversized dispatch: wrong LDS split refusal: Vulkan dispatch: the LDS of 1x65536x4294967295 workgroups of 53256 bytes each is over maxStorageBufferRange 4294967295

On my earlier run of 6a189b3df, main's source passed this test too; the new case is what makes the difference here.

The X split is still not exercised on this GPU: oversized dispatch X split: skipped, the device reports a 2147483647 workgroup limit, which never splits a 129199x1x1 dispatch. The Y split runs.

Full ctest -j 8 on this tree: 1854/1858, the same four as unchanged main on this machine: agc_driver_image_array_window_past_layer_limit is not built (its alignas(65536) hits MinGW's 8192 limit on current main), and the three agc_driver_sample_order* tests that @milescoviello reported on #6683 also fail here.

AI-assisted: yes (runs done with Claude Code).

@milescoviello

Copy link
Copy Markdown
Contributor

@Neithennn, about the three agc_driver_sample_order* failures: they come from the tests, not from this PR. The tests bind the depth surface's own view as a sampled image. When the surface uses #7318's sampled shadow, that image has no SAMPLED usage (VUID-VkWriteDescriptorSet-descriptorType-00337), and the driver returns other depth.

#9056 makes the tests read through a sampled copy, as SampleOrder.cpp itself does. #9057 fixes a separate barrier on the D32S8 copy image that the layer flags (03320).

Results with #9056:

  • RTX 4070 Ti (Linux): all three pass.
  • RTX 3080 (Windows, gdols): two pass, and _grid_reference skips.
  • Radeon 680M: _grid_reference still loses the device, which is a separate problem.

AI-assisted: yes (Claude Code)

@adriadam10
adriadam10 merged commit 60976d8 into boykopovar:main Oct 21, 2026
12 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Re-checked against current main: the early LDS-split refusal in recordDispatch still precedes any allocation (same condition and message, cpuArguments/cpuReason context kept, late check removed). The z = UINT32_MAX case fails on main's sources, as measured by @LeandroLP23 (Radeon 860M) and @Neithennn (RTX 4080); merged.

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

Labels

agc libSceAgc* fix error Closes an issue (Fixes #N)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AGC: an unsupported LDS dispatch split allocates scratch before refusing it

6 participants