Skip to content

perf(shader): read the slow paths of BDA dword and span reads in loops - #4976

Closed
poonasor wants to merge 1 commit into
boykopovar:mainfrom
poonasor:perf/shader-bda-slow-path-loops
Closed

poonasor wants to merge 1 commit into
boykopovar:mainfrom
poonasor:perf/shader-bda-slow-path-loops

Conversation

@poonasor

@poonasor poonasor commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

What

Follow-up to #4931 (see my correction there): the other half of what a commercial title needs to get past the first main-menu frame on AMD's Windows driver.

AMD's compiler inlines the BDA read helpers at every call site (DontInline is not honoured), so the size of a helper body is paid once per read site. Two bodies are unrolled today:

  • read_bda_dword_bytes* loads the four bytes of a dword with four copies of the byte lookup and load.
  • The fallback of read_bda_span* (the span is not mapped as one aligned piece) calls read_bda_dword_bytes* from four sites, one per dword.

Both become one loop over a function-local index: the byte read shifts each loaded byte into the result, the span fallback inserts each dword with OpVectorInsertDynamic and still stops the invocation when a dword read reports a stop. The in-place path of the span read (f1b2e60) is unchanged. Values, fault records and the order of reads are the same; only the shape of the two helper bodies changes.

Measured with the title on an RX 6700 XT (Adrenalin 25.10, amdvlk64), main 7e181fcc3 plus the title's other fixes from my fork:

I have not measured this change without #4931.

Tested

Windows 11, MinGW-w64 GCC 15.2, AMD RX 6700 XT.

  • agc_shader_memory, "BDA read functions": the byte read function must contain one byte load (four on main), and the span read must call the byte read from one site (four on main). With SpirvBdaRead.cpp from main the test fails with "the looped byte load does not keep the access's coherence"; with the change it passes.
  • ctest -R "^agc_shader|^shader|agc_graphics|agc_driver_bda": 16 of 16 pass (on my fork branch, which is 7e181fcc3 plus this and other commits).
  • The title as above.
  • python tools/check_conventions.py --base origin/main passes.
  • This branch itself (the single commit on c2565f014) was not built; the cherry-pick was clean.

Not tested: NVIDIA, Intel, Linux, macOS; whether other drivers gain or lose compile time or run time from the loops.

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 (measured together with perf(shader): read adjacent BDA dword loads of one block through one span read #4931; both touch ShaderMemory.cpp's "BDA read functions" test, so the second to merge needs a rebase)
  • AI-assisted: yes

🤖 Generated with Claude Code

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

Copy link
Copy Markdown
Contributor

Independent build/test of the standalone PR head a4a0f6cda570661c76159bad4df6e1da2239d95c on Linux, without the title integration or #4931.

Device Focused test set
RTX 3090 (Ampere), NVIDIA 595.99.02 18/18 pass
gfx1036 RDNA2 iGPU, RADV Mesa 25.2.8 18/18 pass
llvmpipe (LLVM 20.1.2), Mesa 25.2.8 18/18 pass

The set is agc_shader_memory, agc_driver_bda_device, agc_driver_bda_span_reads, agc_driver_pixel_interlock, and agc_driver_flat_apertures_0 through _13. Serial ctest, 30 s per-test bound, no skips or timeouts. The two device-loss tests from #4975 pass on this NVIDIA/Linux configuration; that does not establish a fix on the failing Windows drivers, and I did not run a matched-parent comparison here.

Each leg restricts VK_DRIVER_FILES to one ICD and sets ANYPS5_GPU, ANYPS5_REQUIRE_VULKAN=1, and ANYPS5_NO_SHADER_CACHE=1. Verbose native logs identify the intended device. The BDA-device helper therefore cannot silently select NVIDIA during the RADV/lavapipe legs.

Ubuntu 24.04, Linux 7.0, Ryzen 9 9950X, GCC 13.3, Release/Ninja, BUILD_TESTING=ON, ANYPS5_ENABLE_SPIRV_TOOLS=ON, bundled dependencies. XRandR headers/link files were supplied through a private extracted prefix; no system package installation or source/test edits. The worktree is clean. Logs and build results are retained.

No title run, NVIDIA driver-cache coldness control, full-suite run, Windows/macOS/Intel test, or compile/runtime performance measurement. This adds standalone Linux/NVIDIA/RADV correctness coverage, not validation of the claimed title speedup.

@xXPBSXx

xXPBSXx commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Windows test (MinGW GCC 15.2, Release, Windows 10, RTX 4080 SUPER). Built on main 534451426 together with six other open GPU PRs (their own commits applied, no merge conflicts), so only the tests that belong to this PR say something about it:

  • tests/ShaderMemory.cpp (target agc_shader_memory_tests), which ctest runs as agc_shader_memory, agc_shader_bindless, agc_shader_srt_read_patching, agc_shader_frontend_pair and agc_shader_frontend_pair_off: 3 of 3 runs passed for each.

Not tested: no game run. The batch was built once, not per PR; nothing failed, so no bisect was needed.

AI-assisted (Claude Code).

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

poonasor commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebased onto main 2f969f147 (it conflicted with 8c04efc, which moved the in-place loads into load_bda_span*).

One change beyond the conflict: the span fallback loop no longer keeps its index and result in function-local variables. main's "BDA read functions" test now requires that a span read function contains no OpLoad/OpAtomicLoad at all, and the variables were loaded there. The loop carries both values as OpPhi in its header instead; the in-place path still goes through the one load_bda_span* call.

Tested on the rebased commit (Windows 11, MinGW-w64 GCC 15.2, RX 6700 XT):

  • agc_shader_memory, agc_shader_bindless, agc_shader_srt_read_patching, agc_shader_frontend_pair, agc_shader_frontend_pair_off: pass.
  • agc_driver_bda_span_reads, agc_driver_bda_device (GPU execution): pass.
  • python tools/check_conventions.py --base origin/main: passes.

Not re-run on this commit: the rest of the suite, and the title (the 140 s link number in the description is from the variable-based loop on 7e181fcc3).

AI-assisted: yes.

@poonasor
poonasor force-pushed the perf/shader-bda-slow-path-loops branch from a4a0f6c to e7249d3 Compare October 13, 2026 11:54
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 13, 2026
@simonbbby

Copy link
Copy Markdown
Contributor

Retested the new OpPhi-based span fallback at exact head e7249d38a1058549f6e2f2c1cb77ddb10875e3bf, standalone on Linux (Ubuntu 24.04 / Linux 7.0, GCC 13.3, Ninja Release, SPIRV-Tools ON).

Device Focused CTest result
RTX 3090, NVIDIA 595.99.02 22/22 pass, 23.91 s total
gfx1036 RDNA2 iGPU, RADV Mesa 25.2.8 22/22 pass, 10.10 s total
llvmpipe LLVM 20.1.2, Mesa 25.2.8 22/22 pass, 49.93 s total; narrower execution coverage below

The set contains the five agc_shader_memory_tests modes named in the rebase comment, agc_driver_bda_device, agc_driver_bda_span_reads, agc_driver_pixel_interlock, and agc_driver_flat_apertures_0 through _13. Each leg pins one ICD with VK_DRIVER_FILES, ANYPS5_GPU, ANYPS5_REQUIRE_VULKAN=1, and ANYPS5_NO_SHADER_CACHE=1; verbose logs confirm the selected device. Serial execution, 30 s per-test limit, no failures or timeouts.

Production-only negative control: replaced SpirvBdaRead.cpp with its exact parent version from 2f969f147b3cb54f63fc4e4e08a5a32456450ca6, retaining/rebuilding the PR's test. agc_shader_memory fails with BDA read functions: the looped byte load does not keep the access's coherence. Restored the PR file byte-for-byte, rebuilt the test and driver, then ran the matrix above. The checkout is clean.

Coverage detail: lavapipe's agc_driver_bda_device returns success while reporting that BDA read/multi-dword write execution is not tested; the fourteen flat-aperture modes omit wave64 rows on its subgroup size 8. These are not CTest-level skips. The same limitations appear in my earlier lavapipe logs, so that earlier pass count should also be read with this qualification. Neither hardware leg printed those exclusions.

No title run, full-suite run, driver-cache coldness control, or compile/runtime speedup measurement. This verifies the rebased implementation's focused Linux correctness, without establishing a fix for the failing Windows configurations in #4975.

@kr4y4

kr4y4 commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

First NVIDIA and Intel run, which the description lists as not tested. The BDA read tests pass on both GPUs. That includes NVIDIA 595.97, which lost the device with the read shape from #4759 until #5022.

Setup: Windows 11 (26200), WinLibs GCC 15.2.0 r7, Release, CI configure flags, an RTX 4070 Laptop GPU (NVIDIA 595.97) and an Intel Raptor Lake-S iGPU (32.0.101.6790). The head e7249d3 merged cleanly into main 0f2683c, which includes #5022 and passes 1069 of 1069 on both GPUs.

  • 25 tests, --repeat until-fail:3: agc_shader_memory, agc_driver_bda_span_reads, agc_driver_pixel_interlock, agc_driver_bda_device, agc_driver_scalar_wide_loads, agc_driver_flat_apertures_0 to 13, and the six flat_loads and global_loads tests. All 25 pass on both GPUs.
  • The same 25 pass with perf(shader): read adjacent BDA dword loads of one block through one span read #4931 merged as well. The two merge with each other without conflicts, ShaderMemory.cpp included.
  • Full suite: 1069 of 1069 on both GPUs.
  • With SpirvBdaRead.cpp from main, agc_shader_memory fails on both GPUs with BDA read functions: the looped byte load does not keep the access's coherence, and the other 24 pass.

I didn't measure compile time or run time.

Written with AI assistance.

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

The diagnosis is right. AMD's LLPC/amdvlk (like Mesa's NIR) inlines every SPIR-V function regardless of DontInline, so helper body size is multiplied by the number of call sites, and a 510K-word pixel shader with 34 span reads hits that badly.

One run-time point to check before this lands, since the change trades unrolled straight-line code for a loop with a dynamic index:

  • OpVectorInsertDynamic with a loop-variant index. If the driver doesn't unroll the 4-iteration loop (and if it did, the compile-time gain would mostly disappear), a dynamic index into a uvec4 is lowered either as a select chain or, on some compilers, by spilling the vector to scratch memory and indexing it there. Scratch use in a pixel shader costs memory bandwidth and can reduce occupancy for the whole pipeline, not only for the fallback path. Building the result with a constant-index insert per iteration (OpCompositeInsert inside a switch on the index), or shifting into scalars and assembling the vector after the loop, keeps the loop and avoids the dynamic index.
  • To confirm: the pipeline statistics that LogPipelineStatistics prints (VK_KHR_pipeline_executable_properties: scratch bytes and VGPRs) for the NHL 27 main-menu pipeline with and without this change on the RX 6700 XT, plus one NVIDIA or RADV device. A frame-time A/B would also show whether the menu frame got slower once it compiles.

The fallback is the rare path (spans not mapped as one aligned piece), so the run-time cost may well be negligible. The point is to show it, since perf( changes to shader emission affect every title's shaders.

@simonbbby

Copy link
Copy Markdown
Contributor

Refreshed the Linux run for new head 3ab89c3e52c1bc1b97ed01f747f5bbec3c8dc97d, standalone on its recorded parent ce5c8395333fbd4bfacdc35065f886333162caa5; no extra source changes.

Release, GCC 13.3, bundled dependencies, SPIRV-Tools ON. Same 22-test set as my earlier report: the five shader-memory/bindless/SRT/frontend modes, BDA device and span reads, pixel interlock, and flat-aperture modes 0..13.

GPU Result
RTX 3090, NVIDIA 595.99.02 22/22 pass, 10.04 s
gfx1036 (1002:13c0), RADV Mesa 25.2.8 22/22 pass, 6.66 s

Each leg pinned its ICD, selected its GPU explicitly, required Vulkan execution and disabled the application shader cache. Serial CTest, unchanged 30-second limit. No internal skips or “not tested” messages in either log. These elapsed times are test-run totals, not a performance comparison.

Repeated the production-only negative control against this head's own parent: restoring only SpirvBdaRead.cpp from ce5c8395333 while retaining the PR test makes agc_shader_memory fail with BDA read functions: the looped byte load does not keep the access's coherence. Restored the PR file byte-for-byte, rebuilt, then ran the GPU matrix above. Checkout is clean.

Lavapipe was not rerun for this head. No title run or pipeline-performance measurement in this report.

A dword read that falls back to bytes called the page lookup once per
byte, and a span read that falls back to dwords called the dword reader
four times. AMD's Windows compiler inlines these helpers at every call
site, so a pixel shader with 232 span reads grew past what it can
compile: 60 GB of memory within a minute for one NHL 27 menu shader.
Both fallbacks are now loops with one call each.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@poonasor
poonasor force-pushed the perf/shader-bda-slow-path-loops branch from 3ab89c3 to 9353192 Compare October 14, 2026 04:40
@github-actions github-actions Bot added the needs rebase Conflicts with main, rebase needed label Oct 14, 2026
@adriadam10 adriadam10 self-assigned this Oct 14, 2026
@adriadam10

Copy link
Copy Markdown
Collaborator

Closing as a duplicate of merged work: #3655 (merged today, 2d3a970) already puts both changes on main.

  • EmitBdaByteLoop in SpirvBdaRead.cpp loops the byte read over one EmitBdaByte, carrying index and result in OpPhi (this PR does the same with function-local variables).
  • The span read fallback in DefineBdaSpanReadFunction already loops over the four dwords with OpPhi and OpVectorInsertDynamic, calling the byte read from one site and stopping the invocation on a stop, same as here.

The PR now conflicts in SpirvBdaRead.cpp and ShaderMemory.cpp, and once resolved against main nothing is left to add. If you still see a compile-time gap on NHL 27 with main as it is, a measurement on current main (and a different change, e.g. for the dynamic insert CnRJay raised) would be welcome as a new PR.

@adriadam10 adriadam10 closed this Oct 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* needs rebase Conflicts with main, rebase needed shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants