Skip to content

perf(shader): probe 32-bit BDA stores and share the write fallback - #1282

Closed
Swastik-51 wants to merge 5 commits into
boykopovar:mainfrom
Swastik-51:perf/probe-bda-stores
Closed

Swastik-51 wants to merge 5 commits into
boykopovar:mainfrom
Swastik-51:perf/probe-bda-stores

Conversation

@Swastik-51

@Swastik-51 Swastik-51 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Depends on #1280 (which builds on #1265). Until they merge, this branch includes their commits; the last commit adds the 32-bit probe and fallback sharing.

32-bit BDA stores previously emitted both an aligned 4-byte store and four unaligned byte stores at every store site, each with a lookup and a page-write probe sequence. With note_bda_write inlined by drivers, programs like agc_driver_flat_store ballooned into dozens of lookups, hundreds of atomic CAS probes, and over 134K GEN instructions on Intel.

Probe the 4-byte span once with Write permission, storing directly when mapped and aligned. The unaligned and faulting fallback is shared across write sites via write_bda_dword_bytes, marked DontInline. note_bda_write is also marked DontInline.

Closes #1256 .

What

  • Probe 32-bit stores: Defined probe_bda_write_pointer (recordFaults = false, BdaAbi::Write) to probe the 4-byte span once. When mapped and 4-aligned, EmitBdaStore emits a single 4-byte store directly and notes the write.
  • Shared fallback function: Defined write_bda_dword_bytes (with a coherent variant) marked with spv::FunctionControlDontInlineMask to handle unaligned accesses, cross-range accesses, and faults without duplicating per-byte lookups at every store site.
  • Prevent inlining of note_bda_write: Changed spv::FunctionControlMaskNone to spv::FunctionControlDontInlineMask in DefineBdaNoteWrite so drivers do not inline its atomic CAS probes into dozens of call sites.
  • Added test: Added verifyBdaWriteFallbackFunctions to ShaderMemory.cpp to verify that note_bda_write and write_bda_dword_bytes have DontInlineMask, main has 0 inline get_bda_write_pointer calls, coherence is preserved, and SPIR-V code size delta per site is kept small.

Tested

  • Checked conventions locally: python tools/check_conventions.py --base origin/main passed with 0 errors.
  • Added and registered unit test verifyBdaWriteFallbackFunctions in ShaderMemory.cpp.

Checklist

@hashaam1217

Copy link
Copy Markdown
Contributor

On Linux this does not keep #1265's gain. Intel Iris Xe, Mesa 25.2.8 ANV, applied to 2cc5735, cold caches:

The cause is the DontInline in e63cc30. Mesa inlines every function in Vulkan shaders and does not check that bit: ANV through vk_spirv_to_nir → nir_inline_functions, and RADV through the same pass. So write_bda_dword_bytes and note_bda_write are copied back into every store site, next to the new probe. The SPIR-V gets smaller while the compiled shader does not. Keeping the fallback in a loop, as #1265 does for the byte stores, is what kept it small here.

Measurements and Mesa source links: #1256 (comment)

@oneandonlydean

Copy link
Copy Markdown
Collaborator

DontInline doesn't hold on Mesa: ANV and RADV inline every function in vk_spirv_to_nir, so write_bda_dword_bytes and note_bda_write end up back at every store site and the second flat_store pipeline runs past 7 GiB (hashaam1217's numbers above). Please drop DontInline and keep the shared fallback in a loop, the way #1265 does for the byte stores, so the compiled shader stays small without relying on it.

Then please add numbers for this commit alone on top of #1280: GEN instruction count and compile memory for agc_driver_flat_store, plus a full ctest run.

@Swastik-51
Swastik-51 force-pushed the perf/probe-bda-stores branch from e63cc30 to fe567b5 Compare October 8, 2026 07:57
@Swastik-51

Copy link
Copy Markdown
Contributor Author

Updated commit fe567b52 with the requested changes:

  • Dropped DontInline from note_bda_write: Reverted back to spv::FunctionControlMaskNone in DefineBdaNoteWrite.
  • Removed write_bda_dword_bytes: Dropped DefineBdaDwordWriteFunctions and its unrolled stores.
  • Routed fallback through looping write_bda_bytes: EmitBdaStore now routes unaligned and probe-rejected stores through storeBytes() (write_bda_bytes, in a loop) so that driver inlining does not duplicate byte store sequences across store sites.
  • Updated test: verifyBdaWriteFallbackFunctions in ShaderMemory.cpp now verifies that fallback byte stores route through write_bda_bytes and keep access coherence with 1 store inside the loop.
  • Passes python tools/check_conventions.py --base origin/main with 0 errors.

@LeandroLP23

Copy link
Copy Markdown
Contributor

agc_shader_memory fails at fe567b5, both on its own (base c5e1018) and merged into main 1081602:

BDA write functions: the byte stores do not keep the access's coherence

It is the new verifyBdaWriteFallbackFunctions, in the non-coherent case: write_bda_bytes has 3 OpStores without Volatile, and the check at ShaderMemory.cpp:1450 expects exactly one. The coherent case doesn't run, because the loop stops at the first case. Windows 11, MinGW-w64 GCC 15.2.0, Release; the test doesn't use the GPU.

This PR has no CI checks yet, which is probably why it hasn't shown up. The rest of the suite is unchanged on a Radeon 860M (AMD numbers for the three PRs are in #1256).

Tested with AI assistance (Claude Code).

@Swastik-51

Copy link
Copy Markdown
Contributor Author

Could you please run the Github CI please as i need maintainer approval to run it

@LeandroLP23

Copy link
Copy Markdown
Contributor

I can't approve workflow runs here; GitHub only lets maintainers do that for first-time contributors. You don't need CI for this one, though: the test doesn't touch the GPU, so it reproduces on any build with ctest --test-dir build -R '^agc_shader_memory$' --output-on-failure. Once it passes locally, I can rerun the AMD measurements on the new head if that helps.

@Swastik-51
Swastik-51 force-pushed the perf/probe-bda-stores branch from fe567b5 to 515df44 Compare October 8, 2026 14:29
@Swastik-51

Copy link
Copy Markdown
Contributor Author

Fixed the agc_shader_memory test issue in 515df443.

The test was counting the two function-local loop counter OpStores as non-volatile stores because length > 3u was part of the array-index expression. It now filters length > 3u before counting, so only the physical buffer store is checked.

python tools/check_conventions.py --base origin/main passes with 0 errors.

I unfortunately can't run the full ctest suite or the requested agc_driver_flat_store measurements locally because I don't currently have a C++ build toolchain or an Intel/Mesa ANV environment. In particular, I can't produce the requested GEN instruction count or peak compile-memory measurement.

Could someone with the Intel/Mesa setup rerun the agc_driver_flat_store benchmark on 515df443? If possible, I'd also appreciate a test-suite run on the updated head.

@LeandroLP23

Copy link
Copy Markdown
Contributor

Ran it on 515df443 merged into main 9f4bd71: Radeon 860M (RDNA 3.5), Windows driver 32.0.13058.8001, Windows 11, MinGW-w64 GCC 15.2.0, Release.

Cold AMD timings, measured as in #1256 (fresh executable name per run so the AMD driver cache misses, ANYPS5_NO_SHADER_CACHE=1, median of 3, whole test):

flat_store global_store_addtid global_vcc_base
main 9f4bd71 3.9 s, 0.48 GiB, 128 KiB, 28 VGPRs 3.9 s, 0.43 GiB, 128 KiB 10.6 s, 0.53 GiB, 256 KiB
#1282 515df44 3.3 s, 0.48 GiB, 64 KiB, 25 VGPRs 3.5 s, 0.43 GiB, 128 KiB 8.8 s, 0.53 GiB, 256 KiB

The KiB column is the size of the AMD driver's pipeline cache file for the run. I can't produce the Intel GEN instruction count from here; that still needs someone with an Intel GPU.

Measured with AI assistance (Claude Code).

@Swastik-51

Copy link
Copy Markdown
Contributor Author

Thanks! Great to see agc_shader_memory passing and the full suite results confirming the two failures are pre-existing on main.
The AMD numbers also look good, especially flat_store improving from 3.9s to 3.3s with 25 vs 28 VGPRs and the pipeline cache dropping from 128 KiB to 64 KiB.
That leaves only the Intel/Mesa ANV GEN instruction and compile-memory measurements requested in the review. I don't have access to that environment, so hopefully someone with Intel/Mesa can rerun the benchmark on 515df44.

@hashaam1217

hashaam1217 commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Intel numbers for 515df44, the commit alone on top of #1280, as requested above. Intel Iris Xe (Alder Lake GT2), Mesa 25.2.8 ANV, Linux. Each variant was merged onto this PR's base, c5e1018, because the head currently conflicts with main in SpirvBdaLookup.cpp and SpirvEmitterState.hpp. Cold runs with ANYPS5_NO_SHADER_CACHE=1 MESA_SHADER_CACHE_DISABLE=true APS5_PIPELINE_STATS=1, one test at a time in a cgroup limited to 7 GiB:

agc_driver_flat_store (passes in all three, no spills):

main (c5e1018) #1280 (1dca7b4) this PR (515df44)
First pipeline: SPIR-V words → GEN instructions, compile time 10,897 → 55,929, 18.8 s 7,598 → 27,965, 7.2 s 8,024 → 21,601, 5.0 s
Second pipeline 17,450 → 111,860, 74.2 s 11,118 → 53,453, 27.0 s 11,229 → 41,175, 18.0 s
Wall time 93.3 s 34.4 s 23.1 s
Peak RSS 7.05 GiB 2.11 GiB 1.38 GiB

Other store-heavy tests. Both fail on this device for the subgroup reason fixed by #1185, which is not in this base; the compile numbers still apply:

main #1280 this PR
global_store_addtid GEN instructions 20,550 9,289 6,743
global_vcc_base GEN instructions, peak RSS 33,525 / 67,050, 2.73 GiB 22,326 / 44,652, 1.50 GiB 19,780 / 39,560, 1.36 GiB

Tests at the same base:

So with DontInline gone and the fallback looping, this commit now makes the compiled shader smaller on Mesa ANV than #1280 alone, not larger as the first version did.

popcoder565 and others added 4 commits October 9, 2026 00:48
Every 8- and 16-bit BDA store and the unaligned path of every 32-bit store emitted one get_bda_write_pointer lookup and one note_bda_write per byte at the store site, five write lookups per 32-bit store. They now call write_bda_bytes, which loops over the bytes and does, per byte and in order, the same lookup, the store when it is mapped and the write note, so faults are recorded exactly as before. On an RTX 3060 the agc_driver_flat_store binaries shrink from 496,256 to 138,240 bytes (wave32) and from 928,384 to 225,152 bytes (wave64), and the cold run from 19.7 s to 2.2 s.
note_bda_write unrolled its 8 compare-exchange probes, and drivers inline it at every BDA store and atomic. The probes now run in a loop that tries the same slots in the same order and stops at the same slot, so the recorded pages and the overflow flag are unchanged. With the byte store helper, the agc_driver_flat_store binaries on an RTX 3060 go from 138,240 to 75,648 bytes (wave32) and from 225,152 to 130,560 bytes (wave64), and the cold run from 2.2 s to 1.2 s.
… path

The declaration, the state field and the definition of write_bda_bytes now sit next to EmitBdaStore and the other BDA write functions instead of next to the dword read functions, which boykopovar#1267 extends at the same lines. No change to the emitted code.
note_bda_write ran a device-scope compare-exchange on every probe, also
when the slot already held the page. The table lives in host-visible
memory, so each BDA store paid an atomic across PCIe, and nearly every
store hits a page an earlier store noted. Load the slot first and only
compare-exchange when it does not hold the page. The slots probed, the
pages recorded and the overflow report are unchanged.

DOOM (PPSA99666) presents through a compute kernel that stores about two
million pixels through raw pointers per frame: 248 ms per dispatch
before, 8.1 ms after, on an RTX 5070 Ti.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
32-bit BDA stores previously emitted both an aligned 4-byte store and four unaligned byte stores at every store site, each with a lookup and a page-write probe sequence. With note_bda_write inlined by drivers, programs like agc_driver_flat_store ballooned into dozens of lookups, hundreds of atomic CAS probes, and over 134K GEN instructions on Intel.

Probe the 4-byte span once with Write permission, storing directly when mapped and aligned. The unaligned and faulting fallback routes to the looping byte store helper (write_bda_bytes).
@Swastik-51
Swastik-51 force-pushed the perf/probe-bda-stores branch from 515df44 to 90d6d56 Compare October 8, 2026 19:24
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 8, 2026
@Swastik-51

Copy link
Copy Markdown
Contributor Author

Closing this PR for now because the branch has accumulated significant merge conflicts with the current main, particularly around SpirvBdaLookup.cpp and SpirvEmitterState.hpp.

The implementation and measurements are still useful, and the latest results look good after the requested changes. Rather than resolving the conflicts on top of the current dependency stack, I'll preserve the work and rebase/cherry-pick the relevant changes onto the latest main once the dependent PRs land, then open a clean follow-up PR.

Thanks everyone for the reviews, testing, and especially the Intel/Mesa measurements.

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.

AGC: each 32-bit BDA store emits five write lookups, so agc_driver_flat_store compiles to 134K/269K GEN instructions and takes ~150 s cold on Intel

6 participants