MagicBeanDev/Longshot

Watch

Fork 98

WIP: fix(shader): narrow global stores find their word's page and declare 8/16-bit storage #520

Draft

Amateurwizrd wants to merge 1 commit from Amateurwizrd/Longshot:fix/narrow-store-spirv into main

Amateurwizrd commented

2026-11-14 17:37:05 +00:00

Contributor

Why

A shader's 8- or 16-bit store to the game's memory could land in the wrong 32-bit word, or be dropped in a page's last
three bytes, and every module that made 8- or 16-bit views of a storage buffer was invalid SPIR-V that drivers happened
to accept: a stricter driver or the validator could reject those shaders.

What changed

shader: the compare-exchange loop for narrow global stores in EmitContext::DefineGlobalMemoryFunctions
(gpu/vk/eden/shader_recompiler/backend/spirv/spirv_emit_context.cpp) looks up the aligned word's address in the page
table (addr & ~3); the bit offset in the word still comes from the byte's own address.
shader: DefineStorageBuffers declares StorageBuffer8BitAccess with SPV_KHR_8bit_storage, and
StorageBuffer16BitAccess with SPV_KHR_16bit_storage, where it makes the u8/s8 and u16/s16 views
(DeclareNarrowStorage, new).
tests: tests/test_narrow_global_store.py and tests/narrow_global_store_ir.cpp (new) run the real emitter on
hand-built modules and read the SPIR-V: the loop's lookup takes addr & ~3; an 8-bit and a 16-bit storage view each
declare their capability and extension and nothing of the other size; narrow global stores alone declare nothing.

Flags and settings

None.

Measurements

main: 9c9d69ed (2026-11-14)
this PR: fix/narrow-store-spirv @ 9ab36d7b
Hardware: RTX 5070 Ti 16 GB, driver 621.01, Ryzen 7 7800X3D, 32 GB, Windows 11 Pro 26200
Scene: none (game-free check)
Method: the new test, run 2 times on each side, interleaved (main, this PR, main, this PR); main gets this PR's test files only
How to reproduce: python -I -B tests/run_all.py narrow_global_store on this branch, and on main with tests/test_narrow_global_store.py and tests/narrow_global_store_ir.cpp copied in

	main @ `9c9d69ed`	this PR @ `9ab36d7b`
tests/test_narrow_global_store.py (run 1 / run 2)	FAIL / FAIL	pass / pass

main @ 9c9d69ed:
FAIL: loop-page-word: the page-table lookup takes the byte's own address, not its aligned word
FAIL: the narrow store modules
FAIL        17.0s  tests/test_narrow_global_store.py
this PR @ 9ab36d7b:
pass        17.0s  tests/test_narrow_global_store.py
1 passed, 0 failed, 0 skipped

Testing

Tested: python -I -B tests/run_all.py on Windows 11 (llvm-mingw): main @ 9c9d69ed 531 passed, 5 failed; this PR
531 passed and the same 5 failing on main (test_oot_state_tap, test_portable_builder_home,
test_portable_builder_startup, test_tested_configurations, test_walk_key), plus test_gpu_hang_watch, which
fails and passes on reruns of main @ 9c9d69ed itself (a 4 s timing bound) and passed alone on this branch. The renderer
builds. In the game, an earlier build carrying this fix together with other changes not in this PR prepared every
program on the RTX 5070 Ti and ran Kokiri Forest and a Hyrule Field walk with no device loss; that run is not a
measurement of this PR.

Not tested: spirv-val over the game's own programs, AMD and Intel drivers, a Linux build (no Linux machine here).

Needs other testers:

AMD (RADV on Linux or Windows) and Intel Arc, Windows: Kokiri Forest and Hyrule Field after the shaders are prepared again, expecting no validation or device-lost errors and the same picture as main (request to open)
Linux (x86_64, gcc or clang): the renderer builds with this PR, expecting no error in the shader recompiler (request to open)

Screenshots

Nothing on screen

Risk and rollback

The translation changes for every program, so each player's shaders are prepared once more after the update. A driver
that lacks 8- or 16-bit storage is unaffected: the views, and so the declarations, are made only where the profile
reports both. Reverting the commit restores the old translation.

Reviewer notes

Found while reading Hookshot-old's 358e7c3f, whose direct narrow store took the capabilities as given; only the two
fixes are here (no direct store), written for Longshot.

Issues

None

Checklist

For everyone

One topic.
The title is a Conventional Commit (type(scope): description).
Rebased on current main.
python -I -B tests/run_all.py run and passing (any failure that also fails on main is named above).
Tested in the game, or what wasn't is named under Testing.
Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests).
Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported.
No game files, dumps, build output or generated files.
third_party/ untouched (a new folder with a PROVENANCE.md instead).
The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled.

If applicable

AI assistance recorded with Assisted-by: / Co-Authored-By: trailers.
A game-free test added for the fix, failing on main and passing here.
Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths. (Windows only so far: Linux is a test request to open)

## Why A shader's 8- or 16-bit store to the game's memory could land in the wrong 32-bit word, or be dropped in a page's last three bytes, and every module that made 8- or 16-bit views of a storage buffer was invalid SPIR-V that drivers happened to accept: a stricter driver or the validator could reject those shaders. ## What changed - shader: the compare-exchange loop for narrow global stores in `EmitContext::DefineGlobalMemoryFunctions` (`gpu/vk/eden/shader_recompiler/backend/spirv/spirv_emit_context.cpp`) looks up the aligned word's address in the page table (`addr & ~3`); the bit offset in the word still comes from the byte's own address. - shader: `DefineStorageBuffers` declares `StorageBuffer8BitAccess` with `SPV_KHR_8bit_storage`, and `StorageBuffer16BitAccess` with `SPV_KHR_16bit_storage`, where it makes the u8/s8 and u16/s16 views (`DeclareNarrowStorage`, new). - tests: `tests/test_narrow_global_store.py` and `tests/narrow_global_store_ir.cpp` (new) run the real emitter on hand-built modules and read the SPIR-V: the loop's lookup takes `addr & ~3`; an 8-bit and a 16-bit storage view each declare their capability and extension and nothing of the other size; narrow global stores alone declare nothing. ## Flags and settings None. ## Measurements main: 9c9d69ed (2026-11-14) this PR: fix/narrow-store-spirv @ 9ab36d7b Hardware: RTX 5070 Ti 16 GB, driver 621.01, Ryzen 7 7800X3D, 32 GB, Windows 11 Pro 26200 Scene: none (game-free check) Method: the new test, run 2 times on each side, interleaved (main, this PR, main, this PR); main gets this PR's test files only How to reproduce: `python -I -B tests/run_all.py narrow_global_store` on this branch, and on main with `tests/test_narrow_global_store.py` and `tests/narrow_global_store_ir.cpp` copied in | | main @ 9c9d69ed | this PR @ 9ab36d7b | |---|---|---| | tests/test_narrow_global_store.py (run 1 / run 2) | FAIL / FAIL | pass / pass | ``` main @ 9c9d69ed: FAIL: loop-page-word: the page-table lookup takes the byte's own address, not its aligned word FAIL: the narrow store modules FAIL 17.0s tests/test_narrow_global_store.py this PR @ 9ab36d7b: pass 17.0s tests/test_narrow_global_store.py 1 passed, 0 failed, 0 skipped ``` ## Testing **Tested:** `python -I -B tests/run_all.py` on Windows 11 (llvm-mingw): main @ 9c9d69ed 531 passed, 5 failed; this PR 531 passed and the same 5 failing on main (`test_oot_state_tap`, `test_portable_builder_home`, `test_portable_builder_startup`, `test_tested_configurations`, `test_walk_key`), plus `test_gpu_hang_watch`, which fails and passes on reruns of main @ 9c9d69ed itself (a 4 s timing bound) and passed alone on this branch. The renderer builds. In the game, an earlier build carrying this fix together with other changes not in this PR prepared every program on the RTX 5070 Ti and ran Kokiri Forest and a Hyrule Field walk with no device loss; that run is not a measurement of this PR. **Not tested:** `spirv-val` over the game's own programs, AMD and Intel drivers, a Linux build (no Linux machine here). **Needs other testers:** - AMD (RADV on Linux or Windows) and Intel Arc, Windows: Kokiri Forest and Hyrule Field after the shaders are prepared again, expecting no validation or device-lost errors and the same picture as main (request to open) - Linux (x86_64, gcc or clang): the renderer builds with this PR, expecting no error in the shader recompiler (request to open) ## Screenshots Nothing on screen ## Risk and rollback The translation changes for every program, so each player's shaders are prepared once more after the update. A driver that lacks 8- or 16-bit storage is unaffected: the views, and so the declarations, are made only where the profile reports both. Reverting the commit restores the old translation. ## Reviewer notes Found while reading Hookshot-old's 358e7c3f, whose direct narrow store took the capabilities as given; only the two fixes are here (no direct store), written for Longshot. ## Issues None ## Checklist ### For everyone - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`). - [x] Rebased on current `main`. - [x] `python -I -B tests/run_all.py` run and passing (any failure that also fails on `main` is named above). - [x] Tested in the game, or what wasn't is named under Testing. - [x] Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests). - [x] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. - [x] No game files, dumps, build output or generated files. - [x] `third_party/` untouched (a new folder with a `PROVENANCE.md` instead). - [x] The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled. ### If applicable - [x] AI assistance recorded with `Assisted-by:` / `Co-Authored-By:` trailers. - [x] A game-free test added for the fix, failing on `main` and passing here. - [ ] Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths. (Windows only so far: Linux is a test request to open)

Amateurwizrd added 1 commit

2026-11-14 17:37:05 +00:00

fix(shader): a narrow global store's page-table lookup by its word, and 8/16-bit storage declared where used

CI / Conventional Commit title (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions

Details

CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions

Details

Third-party parts / As their notes say (pull_request) Blocked by required conditions

Details

Third-party parts / The same as upstream (pull_request) Blocked by required conditions

Details

PR description / Description follows the template (pull_request_target) Successful in 4s

Details

Website / Website on Cloudflare (pull_request_target) Has been skipped

Details

9ab36d7b22

Two faults in the translation of 8- and 16-bit stores to global memory:
the compare-exchange loop looked its 32-bit word up in the page table by the
byte's address, not the word's, so a store to bytes 1-3 of a word could be
aimed past the word; and a module that made 8- or 16-bit views of a storage
buffer never declared `StorageBuffer8BitAccess`/`StorageBuffer16BitAccess` and
their extensions, which is invalid SPIR-V the drivers happened to take. Both
are fixed for every program. The translation changes, so the shaders are
prepared once more.

Found while reading Hookshot-old's 358e7c3f (PR 2976), whose direct narrow
store took the capabilities as given; written anew for Longshot.

Tested: tests/test_narrow_global_store.py (new), tests/test_amd_subgroup_profiles.py,
the renderer builds.

Assisted-by: Claude