Skip to content

perf(shader): intern SPIR-V type and constant ids without a per-call allocation - #327

Merged
adriadam10 merged 6 commits into
boykopovar:mainfrom
slick-daddy:opt-2
Oct 7, 2026
Merged

adriadam10 merged 6 commits into
boykopovar:mainfrom
slick-daddy:opt-2

Conversation

@slick-daddy

@slick-daddy slick-daddy commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

What

OpConstantTrue and OpConstantFalse carry no operand word, so they reached the
declaration cache through the variadic Constant template, which builds the key
vector, and paid an allocation on every call. ConstantBool emits both, so a
shader paid it twice per boolean constant. Add the two-word overload; the
compile-shaped benchmark drops from 10413 allocations to 415.

Add agc_spirv_module_tests covering the interning contract: a repeated
declaration returns one id and is emitted once, operand shapes that share a
leading key word stay distinct, integral and enum operands agree, decorated
types stay separate from plain ones, memoized ids survive AllocateId, modules
do not share ids, and ids stay unique and contiguous past the cache capacity.
The test passes against both the previous header and this one, so it pins the
contract rather than the implementation.

Benchmark, pre-optimization header against this one, best of five:

hot loop x 2M 613.6 ms -> 46.3 ms 13.2x
compile-shaped 5000 instr 4.2 ms -> 0.4 ms 10.8x
64 compile-shaped modules 286.0 ms -> 25.4 ms 11.3x

Allocations for the compile-shaped case fall from 55217 to 415. Both builds
compute the same value over the same call sequence. Measured before this branch
was rebased onto current main; the surrounding call sites have changed since,
so read the ratios as indicative.

Tested

Windows 11. agc_spirv_module_tests built and run standalone (SpirvModule.cpp
plus tests/SpirvModule.cpp, no other dependencies) on MSVC 19.44 /W4 and
MinGW GCC 15.2 with -Wall -Wextra; all ten cases pass on both. The GCC build
reproduces the lookup-order error the previous revision had at the three
requires clauses and compiles clean once the helpers move above them. Full
CMake build and ctest not run locally. No games tested.

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
  • AI-assisted: yes

@github-actions github-actions Bot added build shader core/shader agc libSceAgc* labels Oct 2, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Nice speedup, and I checked that the cache can only ever answer with an id the declarationIds map already holds, so the output stays identical. It doesn't compile with GCC, though: SpirvModule.hpp:62: error: 'wordSized' was not declared in this scope (also at lines 70 and 102). wordSized is declared in the private section further down, and a requires clause, unlike a function body, is not a complete-class context, so the name has to be declared before the overloads that use it. Moving wordSized and narrowWord above the public overloads (or making wordSized a namespace-scope concept) fixes it. CI never ran on this PR, so it didn't catch it. Two more things: agc_spirv_module_tests is added with EXCLUDE_FROM_ALL and no add_test, so ctest never builds or runs it; could you add it to the foreach(agcTest ...) list next to agc_structurizer_tests? And please fill in the Tested section and set the last checklist line to yes or no. With those it looks ready to merge from my side.

slick-daddy and others added 4 commits October 4, 2026 15:58
…allocation

Every SpirvModule::Type and Constant call built a heap std::vector key and
looked it up in a std::map keyed on std::vector, so the comparison walked the
key bytes. At the one to three such calls per emitted instruction a shader
issues thousands of them per compile.

Add a fixed-size, linearly probed declaration cache that holds the key inline
and is consulted before the key vector is built, making a repeated declaration
a hash and a compare. The cacheable shapes are those whose operands are a
word: OpTypeVoid, OpTypeBool, OpTypeInt, OpTypeFloat, OpTypeVector,
OpTypePointer, OpTypeStruct, OpTypeArray, OpTypeFunction and the scalar
OpConstant family. The overloads are constrained on word-sized operands so
that enum operands such as spv::StorageClassFunction reach the cache instead of
binding the variadic template by reference.

Measured with a harness driving the module through the emitters' call shapes:
80000 hot type and constant calls drop from 80074 heap allocations to 140. The
assigned ids and the emitted SPIR-V word stream are byte-identical.

Co-Authored-By: Claude <noreply@anthropic.com>
…test

OpConstantTrue and OpConstantFalse carry no operand word, so they reached the
declaration cache through the variadic Constant template, which builds the key
vector, and paid an allocation on every call. ConstantBool emits both, so a
shader paid it twice per boolean constant. Add the two-word overload; the
compile-shaped benchmark drops from 10413 allocations to 415.

Add agc_spirv_module_tests covering the interning contract: a repeated
declaration returns one id and is emitted once, operand shapes that share a
leading key word stay distinct, integral and enum operands agree, decorated
types stay separate from plain ones, memoized ids survive AllocateId, modules
do not share ids, and ids stay unique and contiguous past the cache capacity.
The test passes against both the previous header and this one, so it pins the
contract rather than the implementation.

Benchmark, pre-optimization header against this one, best of five:

  hot loop x 2M              613.6 ms ->   46.3 ms   13.2x
  compile-shaped 5000 instr    4.2 ms ->    0.4 ms   10.8x
  64 compile-shaped modules  286.0 ms ->   25.4 ms   11.3x

Allocations for the compile-shaped case fall from 55217 to 415. Both builds
compute the same value over the same call sequence.

Co-Authored-By: Claude <noreply@anthropic.com>
@slick-daddy

Copy link
Copy Markdown
Contributor Author

Fixed, and the branch is now rebased onto current main — it was 339 commits behind, which is why the foreach list had drifted.

  • wordSized and narrowWord moved into a leading private: block above the public overloads. They stay private, there are no new namespace-scope names, and the diff is a pure move. Verified with GCC 15.2, the same toolchain CI uses: the previous revision fails with 'wordSized' was not declared in this scope at 62, 70 and 102, and this one compiles clean under -Wall -Wextra. MSVC 19.44 /W4 still compiles it.
  • agc_spirv_module_tests added to the foreach(agcTest ...) list next to agc_structurizer_tests, so ctest now builds it and registers it as agc_spirv_module.
  • Filled in Tested and set AI-assisted to yes.

The test passes on both compilers. No games tested, and the full CMake build was not run locally.

Two things worth knowing. The Build run for this branch is at action_required, so it still needs approving before it can confirm the CMake registration end to end. And the benchmark table predates the rebase, so treat the ratios as indicative rather than current — I left the numbers as measured rather than re-running them against the new base.

Unrelated to this PR, and not touched: agc_driver_recompiler_tests (tests/Recompiler.cpp) is also missing from that same foreach list, so it is built but never registered with ctest.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Thanks, all three are done, and the first two commits came through the rebase unchanged. Ready to merge from my side once CI has run on this head.

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

Copy link
Copy Markdown
Collaborator

Gated on PPSA21564 three times after the shader caches refilled: no visual or FPS change.

One thing since: #611 (mine) added its test next to yours in core/libs/prx/libSceAgcDriver/CMakeLists.txt, so this now needs a one-line rebase keeping both entries. Sorry for the churn.

# Conflicts:
#	core/libs/prx/libSceAgcDriver/CMakeLists.txt
@slick-daddy

Copy link
Copy Markdown
Contributor Author

All done and ctest is 289/289.

@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 5, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Thanks. Your four commits are unchanged from e31f3edf4, the head I gated, and the merge commit's resolution of core/libs/prx/libSceAgcDriver/CMakeLists.txt keeps both agc_spirv_module_tests and main's agc_masked_select_tests. It merges cleanly with current main too. The CI runs for this head are waiting for workflow approval.

One small nit, not blocking: the resolved foreach(agcTest IN ITEMS ...) line lost its four-space indent inside if(BUILD_TESTING).

@slick-daddy

Copy link
Copy Markdown
Contributor Author

Done.

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

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.

Pull request Conflicting files (lines on main) Same exports Same new files
#1046 core/libs/prx/libSceAgcDriver/CMakeLists.txt L519
#1068 core/libs/prx/libSceAgcDriver/CMakeLists.txt L519

@adriadam10

Copy link
Copy Markdown
Collaborator

Checked that the cache only returns ids declarationIds already handed out (including after it fills up), built and ran ctest merged onto current main (396/396), and measured the header on a mixed Type/Constant microbenchmark (GCC -O2: 50 ms → 7.1 ms, identical output). Merging once CI is green.

@adriadam10
adriadam10 merged commit e79c971 into boykopovar:main Oct 7, 2026
3 checks passed
@slick-daddy
slick-daddy deleted the opt-2 branch October 7, 2026 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* build shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants