Repository navigation
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
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.
|
|
Intel and AMD Windows data: the 15 shader tests this PR touches pass on an Intel UHD 770 and an AMD RX 9070 XT. Windows 11 Pro 26300, WinLibs GCC 15.2.0 r7, Release, every execution test built,
AI-assisted: yes (Claude Code) |
|
First AMD-on-Linux (RADV) run of this PR. Merged onto 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 (
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 |
|
First AMD-on-Linux (RADV) run of this PR. Merged onto 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 ( Comparing the JUnit output with 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 |
ahmadmuj
left a comment
ahmadmuj
left a comment
There was a problem hiding this comment.
Reviewed head 30d44b39a merged onto current main a5a780eaa (clean merge). Linux, GCC 13.3, Release, built only agc_shader_memory_tests. CPU side only; the new case stops at SPIR-V, so no GPU needed.
What I ran:
agc_shader_memoryon the merge: passes, 3 of 3.- Main's
ResourceTracker.cppwith your test: fails withGetImageResource dword 0 is not a valid runtime value; chain: Phi(Immediate, LoadAddressU32@pc=0x00000058(...)). pc 0x58 is the news_load_dwordx8, so the red run really comes from the new case. - Same merge with
-DANYPS5_ENABLE_SPIRV_TOOLS=ON(what the Linux CI job configures): passes, so the module from the new case gets through validation. tools/check_conventions.py --base origin/main: passes.
One gap: the loaded arm now gets read even when its branch is skipped.
The copied LoadAddressU32 lands in the merge block, so capture evaluates it on both paths. I ran your shader again with rawUserData[7] = 0, so s_cbranch_scc1 jumps over the s_load:
rawUserData[0] |
Result |
|---|---|
0x500000 (mapped) |
Capture holds both T#s, including 0x21080... read from memory the shader never loads on this path |
0x900000 (nonzero, outside rawRegions) |
CaptureResources and Recompile throw EvaluateRuntimeSources failed to evaluate runtime sources: descriptor source 1 dword 0: LoadAddressU32(...) |
0 |
Works (null root); the second T# reads back as zero |
Main treats this as wrong elsewhere: verifyGuardedDescriptorSources requires a skipped T# to stay null ("a valid T# behind a skipped branch was read", tests/ShaderMemory.cpp:1495 on main). With this PR, a draw that takes the literal arm while s[0:1] holds a stale nonzero pointer fails on the CPU side, even though the shader never touches that address. Main fails this shader for every input, so this isn't a regression, just a part the fix doesn't cover yet.
Could the copied read stay under its arm's condition, the way guarded sources already do? Or, if reading it on both paths is intended, could the test add the s7 = 0 case with an unmapped pointer so the behaviour is pinned down?
Not checked: running it on a GPU (I only have lavapipe, and the new case doesn't dispatch), and whether the copied read also runs unconditionally in the generated SPIR-V.
AI-assisted: yes
30d44b3 to
97b4b5d
Compare
|
@ahmadmuj I reproduced both skipped-arm failures and fixed them in Copied scalar-address reads now retain their original block dependencies. Both the SRT tape and interpreted evaluators apply the existing control-flow walk before evaluating those descriptor sources. The copies are also planning-only, so they do not introduce an additional GPU memory access. The guard metadata survives plan-cache serialization, with invalid block references rejected; the cache format is now 48. The regression checks On Windows 11, WinLibs GCC 15.2 Release with SPIRV-Tools enabled, the production build and the same 23 focused tests pass on main AI-assisted: yes. |
ahmadmuj
left a comment
ahmadmuj
left a comment
There was a problem hiding this comment.
Re-checked head 97b4b5d63 merged onto main 2e1c72239 (clean merge). Linux x86-64, GCC 13.3, libstdc++, Release, -DANYPS5_ENABLE_SPIRV_TOOLS=ON like the linux CI job. Built agc_shader_memory_tests and agc_shader_disk_cache_tests only.
This head doesn't build on Linux. ShaderDiskCache.cpp:112:
error: static assertion failed: DescriptorSource changed: update the plan encoder
static_assert(sizeof(DescriptorSource) == 192, ...)
That block sits under #if defined(__linux__) && defined(__x86_64__) && defined(__GLIBCXX__), so a Windows build never sees it. The new std::vector<std::uint32_t> requiredBlocks adds 24 bytes, so it needs to be 216. The linux job is skipped on this head, so CI hasn't caught it yet.
With only that line changed locally to 216, the fix itself checks out:
agc_shader_memoryandagc_shader_disk_cachepass, 3 of 3.- Our earlier table is now covered by your new rows. With
s7 = 0and roots0x500000,0x900000and0, the loaded T# stays null, nothing reports a null root or fault, and capture and recompile succeed. The taken unmapped arm still fails withfailed to evaluate runtime sources. - Red runs, each one change on top of this head:
ApplySourceBlockGuardsreturningtrueat the top: fails withdescriptor Phi: a skipped scalar-address T# was read.- Only the
SrtTape::Runguard call removed: same failure, so the tape path is pinned on its own too. encodePlanwriting an emptyrequiredBlocks: fails withdescriptor Phi: the cached plan lost its raw read guard, so the cache round trip is really tested.
tools/check_conventions.py --base origin/mainpasses.
Not checked: a GPU run (the synthetic case stops at validated SPIR-V, and I only have lavapipe), and other open PRs that also bump the cache to 48.
AI-assisted: yes
|
@ahmadmuj Thanks for catching the missed Linux layout assertion and checking the negative controls. Fixed in 6032c49: The Windows relinker/libraries build and the same 23 focused tests passed on both main and the fixed head, with zero skips and no Vulkan validation errors on the RTX 4070 Ti SUPER. Explicitly enabling the Linux-gated assertions under MinGW reproduces the failure at 192 and passes at 216; that is not a native Linux build. I could not rerun native Linux here, so the updated PR description attributes those results to your review. AI-assisted: yes. |
What
An image descriptor merged from a literal T# and a scalar-address load was rejected during preparation with
GetImageResource dword 0 is not a valid runtime value. The tracker can now recreate the loaded descriptor arm when its offset is a memory-independent expression of user data and constants. Pointer reuse still requires dominance and same-block instruction order; memory-derived offsets remain refused after SRT planning.The reviewed skipped-arm case also needs the original read's execution scope. Copied raw reads retain their original block dependencies. The existing resource control-flow walk activates a descriptor only after all its required load blocks are reached, in both the SRT tape and interpreted evaluators. An untaken loaded arm stays null even with a stale nonzero pointer; a taken unmapped read still fails. This uses the existing conservative control-flow evaluation rather than treating arbitrary pointers as valid.
The rematerialized copies are planning-only, so they do not add another GPU memory access. The original shader reads retain their original execution scope. Required block indices are serialized in cached plans, checked when decoding, and included in descriptor-source interning. Cache format advances from 47 to 48.
The regression covers skipped mapped/unmapped/null roots, taken mapped and unmapped roots, zero fault/null-root reports on skipped reads, cache-plan round trips, invalid cached block references, and actual execution of both SRT tape and interpreted source queries. It checks that copied reads stay planning-only. No title compatibility or speedup is claimed.
The Linux x86-64/libstdc++ layout assertion now expects
sizeof(DescriptorSource) == 216: the addedrequiredBlocksvector contributes 24 bytes to the previous 192-byte layout. Encoding, decoding and the aggregate field-count guard already cover that field.Tested
Main
baabfbc2046a850e54e60af3a7fd9949f4669e1d; submitted head6032c492ded9d9c79a1714ce2375a0592cb58f11. Windows 11 Pro 10.0.26300, AMD Ryzen 9 7900X, 31.7 GiB RAM, WinLibs MinGW-w64 GCC 15.2.0 r7 UCRT, Ninja, Release,BUILD_TESTING=ON,ANYPS5_ENABLE_SPIRV_TOOLS=ON, bundled dependencies. NVIDIA GeForce RTX 4070 Ti SUPER, driver 617.42 / 32.0.16.1742; Khronos validation 1.4.363.cmake --build <build> --target relinker libs patched_libraries_tests agc_shader_memory_tests agc_shader_disk_cache_tests agc_shader_replay agc_shader_replay_requests_tests agc_driver_image_table_tests --parallel 8: passed on both exact revisions.agc_shader_memory_tests, plus shader disk cache, both shader replay tests, patched libraries and driver image table. The guarded scalar-address descriptor regression executed on the fixed head.APS5_VERIFY_SRT_TAPE=1,APS5_VERIFY_SRT_COMPACT=1,ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation. Logs confirm the selected RTX and actual instance/device validation-layer insertion; neither reports a missing requested layer,VUID-, validation warning or error. The synthetic fragment regression stops at validated SPIR-V; it does not dispatch that shader on the GPU.-D__linux__ -fsyntax-onlyunder MinGW reproduces216 == 192with the old assertion and passes with 216. This is a Windows compiler check of the guarded code, not a native Linux build.python tools/check_conventions.py --base origin/main,python tools/check_main_sync.py --base origin/main --strict,git diff origin/main...HEAD --check, andpython tools/check_test_registration.py <build>passed.python tools/pr_overlap.py --branch codex/pr7694-review-20261025 --repo boykopovar/AnyPS5 --jobs 12completed.Shared changes remain separate: #7223/#7395 add nested/pointer-keyed tables; #6488/#7642/#10193 add lane/device-table paths; #7684 bounds table keys; #8077 handles folded/invalid descriptor-web arms. Their table, descriptor-fault and cache fields must be preserved alongside this read provenance and its encode/decode validation. #5609/#6539 add source-lead tracing, which belongs after the guard activity is applied. Cache schema/version changes must be reconciled when composing other schema-changing PRs. No peer implementation is carried here and no author-agreed merge order or full mixed-stack validation is claimed.
The size assertion also overlaps #8939/#9457/#9633/#9749/#9790/#9969, which extend
TableColumnand use their own cache schema versions. Composing them must preserve both sets of fields, reconcile the schema version, and recompute the combinedDescriptorSourcesize; neither standalone size may be copied blindly. This assertion fix introduces no new runtime prerequisite.The complete CTest suite, original title shader and title/homebrew run were not executed. The earlier negative controls on production patch
0a362acfc0afailed on skipped mapped and unmapped scalar-address reads; they are historical evidence for the guarded-read fix, not new runs for this assertion update.Exact focused CTest command
Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)agc_driver_image_table)