Repository navigation
perf(shader): keep one use list per IR value - #7520
Open
milescoviello wants to merge 1 commit into
Open
milescoviello wants to merge 1 commit into
milescoviello wants to merge 1 commit into
Conversation
Every IR value kept two lists of its users: uses (the users) and operandUses (the users with their operand index). AddArgument, ReplaceArgument and ReplaceAllUsesWith updated both, and each value allocated and freed both. Only operandUses is kept now. Uses() is a view of the users in operandUses, and HasUses, UseCount and Invalidate read it. sizeof(IrValue) drops from 152 to 128 bytes; the plan value encoder writes no use lists, so only its size check changes. The two lists held the same users in the same order, except after ReplaceArgument on a user that took the value in more than one operand: uses dropped the user's first entry and operandUses the replaced operand's entry. Uses() now follows operandUses there too. shader_ir_use_rewrite counted the allocations of both lists: its allocation failure check now fails the one reserve ReplaceAllUsesWith makes, and its growth check allows the allocations of one list, which main exceeds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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.
|
Contributor
Author
|
A note on how this composes with my other open shader preparation PRs: #7431, #7491, #7494, #7503, #7504, #7516, #7518 and #7522. I merged all nine onto
Release replay CPU time (
Each PR still stands on its own, and its description has its own measurement against AI-assisted: yes (Claude Code) |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Every
IrValuekept two lists of its users.uses(std::vector<IrValue*>) held the users, andoperandUses(std::vector<IrUse>) held each user with the operand index it reads.AddArgument,ReplaceArgumentandReplaceAllUsesWithupdated both, and every value allocated, grew and freed both. In a profile of a Release replay of 355 MXGP 26 (PPSA30839) shader requests, the allocator was 20% of all samples. Destroying IR programs,IrProgram::CreateValueandIrValue::AddArgumentwere its largest callers.Only
operandUsesis kept now:Uses()returns a view of the users inoperandUses(std::views::transform). Its six callers in the recompiler only iterate it or test it withall_of, exceptSsaPass::TryRemoveTrivialPhi, which copied it into a vector and still does.HasUses,UseCountandInvalidatereadoperandUses.ReplaceArgumentno longer searchesuses, andReplaceAllUsesWithreserves and fills one list.sizeof(IrValue)drops from 152 to 128 bytes. The plan value encoder inShaderDiskCache.cppwrites no use lists, so only its size check changes.The two lists held the same users in the same order in all but one case:
ReplaceArgumenton a user that took the value in more than one operand.usesthen dropped the user's first entry, andoperandUsesdropped the replaced operand's entry.Uses()now followsoperandUsesthere as well, which is the listReplaceAllUsesWithandIrUse-based passes already worked from.shader_ir_use_rewritecounted the allocations of both lists, so two of its expectations change:ReplaceAllUsesWith's two reserves in turn. It now fails the one reserve that is left.2 * (bit_width(count) + 1)allocations for 4,096 replacements. It now allowsbit_width(count) + 1.mainfails the new bound ("geometric use-list growth: IR use rewrite assertion failed"), and this PR passes it.Tested
Ubuntu 24.04 (Linux 6.8), GCC 13.3, NVIDIA RTX 4070 Ti, driver 595.99.02.
ctest -R "^agc_|^shader_ir"on 9f25d0b: 709 tests. 708 passed on the first run, with 3 skipped (host import sparse, sparse dma-buf import, stencil ref export).shader_ir_use_rewritefailed on the two expectations above, and passes with them updated. After that change, the 23 tests matching^shader_ir|^agc_ir|^agc_shaderpass.agc_shader_replay --spv(Release,ANYPS5_ENABLE_SPIRV_TOOLSoff,ANYPS5_NO_SHADER_CACHE=1) on 466 shader requests I had dumped withAPS5_DUMP_SHADERS: 355 from PPSA30839, 107 from PPSA31162 and 4 from another app. All 462.spvfiles (a few names repeat across the dumps) have the same SHA-256 onmain9f25d0b and with this PR, and the replay output is identical.Replay CPU time (
perf stat -e task-clock), interleaved:main9f25d0b0x1e401c0000(1,056,287 SPIR-V words), 15 runs each0x10f9f30000(1,130,170 words), 15 runs eachThat is about 13% on the two large shaders and about 9% over the whole set.
Title level: no speed claim. This is background shader preparation. I didn't run a title for it.
This touches the same IR as #7503 (
ReclaimUnusedValues, which only callsHasUses) and #7518 (IrValue's constructor). They don't conflict, and each was measured on its own.tools/check_conventions.py --base upstream/mainpasses.tools/pr_overlap.pylists only conflicting PRs that it could not compare onShaderDiskCache.cpp. #6232, #6946 and #6947 have thesizeof(IrValue)check as a context line and don't changeIrValue.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)🤖 Generated with Claude Code