Repository navigation
Conversation
…time CPU (#482) * perf(shader): rewrite high-fan-out IR uses in linear time Ported from upstream PR boykopovar#4035 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * perf(shader): index specialization tables by SPIR-V id Ported from upstream PR boykopovar#4772 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(shader): honor disabled optimization during specialization Ported from upstream PR boykopovar#4827 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * perf(shader): discard helper functions unreachable after specialization Ported from upstream PR boykopovar#5064 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(shader): drop module constants nothing references after specialization Ported from upstream PR boykopovar#5315 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(shader): cap SPIR-V scalar replacement Ported from upstream PR boykopovar#4288 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * perf(recompiler): iterate structurizer fixpoints in post-order and bound shared merge splitting growth Includes the stacked upstream commit that structurizes refused control flow with reach flags, which this change builds on. Ported from upstream PR boykopovar#5028 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * perf(shader): emit image_bvh_intersect_ray as one shared function per variant Ported from upstream PR boykopovar#5455 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * perf(shader): avoid redundancy traversal map copies Patches the bundled SPIRV-Tools redundancy elimination (only built with ANYPS5_ENABLE_SPIRV_TOOLS=ON). Ported from upstream PR boykopovar#5511 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(metal): accept a specialized BDA module that dropped one of its two BDA descriptors Specialization removes descriptor variables the final module no longer references. With helper functions now discarded after specialization, a compute module can drop its page-table variable while keeping the fault buffer, and the bridge then rejected the dispatch ("BDA ABI requires page-table and fault-buffer descriptor metadata"). Require at least one of the two roles instead of both; the module's own descriptors are still checked against the prepared plan. Not a port; needed by the port of upstream PR boykopovar#5064. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Conflicts with main in |
# Conflicts: # core/libs/prx/libSceAgcDriver/tests/ShaderDiskCache.cpp
|
Still needs the rebase for |
|
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.
|
…ation A specialized pipeline variant materializes its specialization values as OpConstant, and SpecializeSpirv folds them into the code. The folding left the materialized constants in the module with no use. Image channel selectors are such values (RuntimeImageSwizzle). Two variants of one program that differed only in an image's channel order therefore had identical code but different SPIR-V, and the second one was created and compiled again instead of coming from the pipeline cache. removeUnusedConstants now also removes module constants (OpConstant, OpConstantTrue/False, OpConstantComposite, OpConstantNull) that no instruction references any more, repeating until nothing changes. Run already drops the names and decorations of removed ids. Test: agc_shader_disk_cache verifySpecializationDropsDeadConstants. Two modules that differ only in a constant the folding consumes specialize to the same words, that constant and its name are gone, a constant still in use (an array length) stays, the store still reads the selected value, and the result is stable. agc_formatted_buffer_once bounds its vertex registration module by 1.5x the specialized one. Both modules lose dead constants, the specialized one more (Linux with SPIRV-Tools: 1348 -> 1126 words, registration 1917 -> 1739), so their ratio moves from 1.42 to 1.54. The bound moves to 1.6x; a module that kept the format table for every format would be about 14,000 words, like the GPU-selected compute module (13,897).
76bb7da to
1ea471b
Compare
|
@oneandonlydean rebased onto Conflict. Tests (fork, Linux, lavapipe, SPIRV-Tools on, all 605
Sizes in
The 1.6x bound is still needed: 1.5x of 1126 is 1689 words and the registration module has 1739; 1.6x gives 1801. The Cold load. The Sword of the Sea dump the description was written against is gone, so the before/after runs on Minecraft Dungeons II (PPSA16064) at its title screen, one machine (RTX 5070 Laptop 8 GB, NVIDIA 617.42, Windows 11), |
|
The rebase is right, both tests kept. The |
|
Cold-load numbers from Minecraft Dungeons II (PPSA16064) on Windows 11, NVIDIA GeForce RTX 5070 Laptop GPU (617.42): two rows per arm,
So the change does what it says: the two specializations that differed only in constants nothing references now share one module, and the modules are 10 % smaller. The 15.7 s in the PR's first row is the NVIDIA driver compiling the new modules once: the driver keeps its own cache outside the row's AI-assisted: yes (Claude Code) |
|
@AlxcBTW Since your 15 Oct rebase, Merged onto current
With Could you rebase onto current |
…eferences after specialization) at 76bb7da
…onstant of its value boykopovar#6176 aliases every folded result to the first module constant of the same type and value. A select that boykopovar#5315's dead-constant test folds to a copy of one constant then aliased to the variant's own selector constant whenever the two held the same value, so variants that differ only in that folded constant kept it (and its name) and specialized to different SPIR-V (agc_shader_disk_cache). A folded OpCopyObject now aliases to its source constant (or that constant's alias); other folds keep boykopovar#6176's canonical choice. AI-assisted: yes
…ts referenced constants agc_spirv_optimizer (built with ANYPS5_ENABLE_SPIRV_TOOLS, as upstream's Linux job does; build-local has it off) failed on this branch before today's local fixes as well. boykopovar#6176's test counted five module constants: %one and %three stayed because nothing removed unreferenced module constants on its head. boykopovar#5315 now drops them, so the module keeps %two, 7 and 8, the three the stored lane + 23 reads. The test expects 3. AI-assisted: yes
…that cannot fault Upstream 7e4ce1a makes an unaligned 32-bit buffer atomic act on its containing dword, so the specialized atomic records no fault. Upstream keeps the uncalled fault function and with it the fault buffer binding; our boykopovar#5315 specialization removes dead functions, so the module no longer uses the fault buffer, Select drops the binding and bdaAbiVersion is 0 ("the program has no fault buffer for its unaligned atomic"). The test now expects that, and still checks no address tables, the atomic element flag, Int64 and no fault call. AI-assisted: yes
THEMANnt
left a comment
THEMANnt
left a comment
There was a problem hiding this comment.
Reviewed 1ea471b57089 against merge base 03c4cb5e78e0. Recommend approval; no actionable finding in the complete three-file patch.
The iterative cleanup removes unused specialized constants and associated metadata while retaining referenced array lengths and the composite operands preserved by the current visitor. Reviewed the specialization-to-cache/compilation flow and the adjusted formatted-buffer size bound.
Validation: Built agc_shader_disk_cache_tests on candidate and base with Windows GCC 15.3; both CTest runs pass. An additional review-only harness passes checks for dead scalar/bool/null constants, consumed-value/name removal, retained array/composite operands, unchanged input and idempotence. Its initially incorrect composite-removal expectation was corrected before the final passing run; no production source was changed.
Scope: ANYPS5_ENABLE_SPIRV_TOOLS=0, so no independent SPIR-V validator ran. No local Linux/lavapipe execution; hosted product jobs were skipped. The baseline's original-suite pass is not a failing-old-code regression control.
|
Merge-skew check on Linux (i7-8700, Ubuntu 26.04, GCC 15.2, Release). Head
Mechanism (adds to @oneandonlydean's note about #6176 / 3e1f932): Verified minimal fix ( Fix diff (against main)diff --git a/core/shader/recompiler/SpirvBackend/src/SpirvSpecialization.cpp b/core/shader/recompiler/SpirvBackend/src/SpirvSpecialization.cpp
index a3e385452..6caa64369 100644
--- a/core/shader/recompiler/SpirvBackend/src/SpirvSpecialization.cpp
+++ b/core/shader/recompiler/SpirvBackend/src/SpirvSpecialization.cpp
@@ -803,8 +803,12 @@ private:
const auto type = types.at(instruction[1]);
const auto bits = type.width == 32u ? *result : *result & ((1u << type.width) - 1u);
values[instruction[2]] = bits;
- const auto [existing, created] = canonical.emplace(std::pair{instruction[1], bits}, instruction[2]);
- if (!created) aliases.emplace(instruction[2], existing->second);
+ if (op == spv::OpCopyObject && values.contains(instruction[3])) {
+ const auto* shared = aliases.find(instruction[3]);
+ aliases.emplace(instruction[2], shared != nullptr ? *shared : instruction[3]);
+ } else if (const auto [existing, created] = canonical.emplace(std::pair{instruction[1], bits}, instruction[2]); !created) {
+ aliases.emplace(instruction[2], existing->second);
+ }
constants.push_back(type.boolean ? Make(bits != 0u ? spv::OpConstantTrue : spv::OpConstantFalse, {instruction[1], instruction[2]}) : Make(spv::OpConstant, {instruction[1], instruction[2], bits}));
instruction.clear();
changed = true;Written with AI assistance (Claude Code). I ran the builds and tests above on my machine. |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
Follow-up to my merge-skew note above, now with ANYPS5_ENABLE_SPIRV_TOOLS=ON (that build did not have it). Linux, i7-8700, Ubuntu 26.04, GCC 15.2, Release. Head 1ea471b57 merged with --no-ff onto main 8df2c3ed0. lavapipe (Mesa 26.0.3).
The disk-cache fix from my earlier comment does not cover the optimizer test @oneandonlydean mentioned:
agc_shader_disk_cache |
agc_spirv_optimizer (+ _full, _none) |
|
|---|---|---|
main |
pass | pass |
main + this PR |
fail 3/3 | fail 3/3 |
main + this PR + the 6-line SpirvSpecialization.cpp fix |
pass 3/3 | fail 3/3 |
Only one check fails in testSpecializedConstantsAreShared: "specialization kept an unreferenced folded constant or lost a referenced one" (constants.size() == 5u). The module itself is fine: ValidateAndOptimizeSpirv accepts it and every other check in that test passes (sharing, no bitcasts or copies, lane + 23, stable second pass).
The two missing constants are not folded ones. main leaves %one = 1 and %three = 3 from the source in place (it only drops unreferenced constants that the folding itself materialized), so the test expects 1, 2, 3, 7, 8. This PR's sweep also removes original module constants that nothing references, so the output has 2, 7, 8. That is the intended effect of the PR, so it comes down to which behaviour main wants:
- keep the sweep and update the expectation to
3u, or - limit the sweep to what
main'sremoveUnusedConstantsalready removes.
With the disk-cache fix plus 5u → 3u, the ^agc suite on lavapipe with SPIRV-Tools ON (1053 tests, display/flip/lds_atomics64 excluded) matches main: 1051/1053, failing only agc_driver_multisample_retirement and agc_driver_guest_memory_lifetime_converted, which fail on main too.
GPUs tested: lavapipe (Mesa 26.0.3) for everything above. On main, the 10 tests that only exist with SPIRV-Tools ON also pass on an RTX 3070 (NVIDIA 595.99.02).
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
@jotitadev thanks for the merge-skew check and for tracing the mechanism: it confirms the conflict with #6176 that we flagged. @AlxcBTW the fix still needs to keep the folded-constant definitions that undecoded consumers name, so that both |
|
Data from another title on whether #6176 already covers this: Astro Bot (PPSA21564), Linux, RADV, a local build on
The five hottest fragment programs had 36 specialized modules but only 5 distinct codes. For example, two of one program's 21 variants differ only in three |
|
Static review plus an independent Linux/AMD run on both trees per the repo AGENTS shader-recompiler protocol (CachyOS, kernel 7.2.10, GCC 16.2.1, Ninja 1.13.2, Release, What I checked
Focused paired run (AGENTS shader-recompiler protocol)
Both logs: 0 Rebase note This branch is behind |
❌ NVIDIA GPU test: failedPR head
Failing testsThis PR changes test files, so each failing test was also built and run on main with only the PR's test changes applied. ✅ there means the failure comes from the PR's other changes; ❌ means the test fails on main too, so this run does not show the PR caused it.
agc_shader_disk_cache: Failed (also failed when re-run alone), assertion and diagnosticsagc_spirv_optimizer: Failed (also failed when re-run alone), assertion and diagnosticsagc_spirv_optimizer_full: Failed (also failed when re-run alone), assertion and diagnosticsagc_spirv_optimizer_none: Failed (also failed when re-run alone), assertion and diagnosticsFailing on main too (3, not counted against this PR)Validation IDs already present on main (8 IDs in 531 tests)Skipped (50)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Verdictshould-fix Findings
Freshness envelope
Cross-PR ledger
|
github-actions
Bot
added
review queue
BunnyFl
left a comment
BunnyFl
left a comment
There was a problem hiding this comment.
@AlxcBTW the new verifySpecializationDropsDeadConstants fails once this PR is merged into current main (runs below). So on main, the two variants in the test still specialize to different SPIR-V.
What I ran: head 1ea471b570 merged onto main 9ab6a281ea (clean merge, e4a39e6cd3). Windows 11, WinLibs GCC 15.2.0, AMD RX 9070 XT, ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation:
agc_formatted_buffer_oncepasses.agc_shader_disk_cachefails withvariants that differ only in a folded constant specialized to different SPIR-V.- With
main'sSpirvSpecialization.cpprestored, it fails the same way. - No
VUID-errors and no missing-layer lines in either run. - The head alone, unmerged (base
03c4cb5e78):agc_shader_disk_cachepasses. So the failure comes from whatmainhas changed inSpirvSpecialization.cppsince then (13 commits, merges included).
Why, as far as I read it (main 9ab6a281ea):
- At parse time, each scalar module constant registers as the canonical id for its type and value. The first one wins, because it's an
emplace(SpirvSpecialization.cpp#L394). - When
foldturns an instruction into a constant whose value is already canonical, it records an alias to that id (#L623).shareConstants(#L849) then rewrites the uses to the canonical id.Runcalls it (#L428) beforeremoveUnusedConstants(#L431). - In the test module, the selector is
%6and the selected constant is%7 = 4. With selector 4,%6and%7both hold 4, and%6is emitted first. By theemplaceat #L394 above, the foldedOpSelectthen aliases to%6, and the store reads%6. With selector 6, the store reads%7. - So after this PR removes the unused one, the two modules still differ:
OpConstant %3 %6 4plusOpStore %8 %6, againstOpConstant %3 %7 4plusOpStore %8 %7. - Probe: on the same merge, I changed only the test's first variant from
module(4u)tomodule(8u), which gives selector 8, a value no other constant in the test module has.agc_shader_disk_cachethen passes. That fits the explanation above.
The channel selectors from the description (4, 5, 6, 7) are the kind of values that may equal another constant in a real module, so the cache misses may persist on main for those programs. I haven't checked that on a real module. For the fix, one option is to make the canonical id independent of the selector's value. For example, re-number the surviving constants after removal, or prefer the constant that the folded instruction's own operands name. Either way, the test needs to pass on the merge with current main.
Also checked:
pr_overlap.py --branchon the head: no conflicting lines or shared exports. 20 PRs that conflict withmainshare files and couldn't be compared. The only one onSpirvSpecialization.cppis #8004, which moves helpers nearVisitInputsand doesn't touchremoveUnusedConstantsor the constant sharing.- Minor point for the description: it says
OpConstantCompositeis not removed, but the loop does include it in the opcodes it removes. Whether a composite is ever unreferenced depends onVisitIdOperands, which I haven't read through.
github-actions
Bot
added
needs rebase
What
Two pipeline variants of one program that differ only in a folded specialization value now produce the same SPIR-V. The second variant is then served by the pipeline cache instead of a cold compile.
buildSpecializedModulematerializes a variant's specialization values asOpConstant, andSpecializeSpirvfolds them into the code. The folding left the materialized constants in the module with no use.Image channel selectors (
RuntimeImageSwizzle) are such values. So two variants of a program whose only difference was an image's channel order, 4,5,6,7 vs 6,5,4,7, had identical code but different SPIR-V, and the second one was created and compiled again.removeUnusedConstantsnow also removes scalar and null module constants (OpConstant,OpConstantTrue/False,OpConstantNull) that no instruction references any more.OpConstantCompositeis not removed:VisitIdOperandscounts a composite's own result id as a reference, so a composite and its members always stay.Runalready drops the names and decorations of removed ids.Evidence from Sword of the Sea (PPSA25344), loading on a cold cache, compute programs of 100k+ SPIR-V words. Every pipeline's SPIR-V was dumped (
APS5_DUMP_COMPUTE_PIPELINES) and diffed withspirv-dis --raw-id:OpConstantvalues that nothing in the module uses: each id occurs once, at its definition. The values are 4/5/6/7 in another order. The 15th pair is a real variant (10,738 differing lines).Test change: agc_formatted_buffer_once
@adriadam10, this touches the bound from 56d5a49. That bound checks that a formatted load from a descriptor with a known format does not carry every format's decoder in registration form, measured against the specialized module.
This change shrinks both modules, the specialized one more, so the ratio between them moves. Sizes in words, from this PR's fork CI and from
main0214069 on the same runners:mainmainThe bound moves from 1.5x to 1.6x of the specialized module. A registration module that kept every decoder would be about as large as the GPU-selected compute module, about 14,000 words on current
main(13,897; it was 85,945 when this PR was opened, before #3656), so 1.6x still catches it.I first tried bounding the registration module against a vertex module with a GPU-selected descriptor instead, which would have measured the property directly. The recompiler rejects that request in both forms ("buffer specialization has an unsupported format"), so the test keeps its original comparison.
Tested
agc_shader_disk_cache, newverifySpecializationDropsDeadConstants:OpNameare gone;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)