Skip to content

perf(shader): share immutable binding access metadata - #1318

Closed
TibixDev wants to merge 5 commits into
boykopovar:mainfrom
TibixDev:pr/binding-reflection
Closed

TibixDev wants to merge 5 commits into
boykopovar:mainfrom
TibixDev:pr/binding-reflection

Conversation

@TibixDev

@TibixDev TibixDev commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Status after review

Blocked on binding-plan benefit and metadata redesign.

Audited today's types: CompiledShaderArtifact already holds compiled/static shader data, while ShaderInvocation holds specialization, live descriptor bindings, push constants and vertex attributes. The remaining candidate is immutable descriptor-access vectors; live values still have to be materialized per invocation. This is narrower than treating the old complete binding object as shareable.

The serialized cache is now FormatVersion = 27; replaying this branch's old version edit would be incorrect. A replacement needs a new version chosen against its landing base and round-trip/invalidation checks. #1315 still has no demonstrated title benefit, and this stacked head has no isolated modern comparison. No code push or claim that the prior 448 → 160-byte layout result establishes runtime benefit on the new structures.

What

Prepare descriptor access metadata once per compiled shader variant. Share it across draw results. Materialize current descriptors, sampler coordinates, and push data on each use. Save the prepared metadata in versioned shader-cache entries.

Tested

Earlier branch-specific tests and measurements remain in the discussion. They have not been rerun on a redesigned current-main version of this PR. The review audit and shared NGS-assisted title exploration do not establish that this head is merge-ready. No full Linux/Windows platform CI was dispatched in this pass.

Checklist

@TibixDev

TibixDev commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor Author

Rebased on da402739. Preserved current upstream and the #1315 prerequisite. Fixed the Linux CI failure: sampler validation now runs before SPIR-V emission, while immutable binding metadata remains shared. Reproduced the failure with SPIRV-Tools enabled; shader-memory, disk-cache and binding-materialization tests now pass three repetitions in that configuration. All tests below use SPIRV-Tools enabled.

Fedora 44 Release: agc_binding_materialization_tests, agc_shader_disk_cache_tests, agc_shader_memory_tests, agc_driver_tests, agc_driver_graphics_tests, agc_driver_recorder_tests build; the targeted tests and contribution checks pass. Windows CI is pending.
RADV and lavapipe: --binding-plan-only pass with core and synchronization validation; no validation errors.

Shader-memory, disk-cache and binding-materialization also pass with SPIRV-Tools disabled.

@TibixDev
TibixDev force-pushed the pr/binding-reflection branch from 7fbacba to 122e0f2 Compare October 8, 2026 07:35
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 8, 2026
Retain descriptor layouts, allocation counts and buffer patch positions
per shader variant. Resolve live addresses on each resource build and
preserve independent resource lifetimes. Bound plan retention and bypass
unknown variant identities.

Cover changing addresses, array bindings, push offsets, eviction and plan
lifetime. Supply the sampled-image metadata required by the resource-build
benchmark. Fedora 44 Release driver, graphics and PM4 tests pass. RX 6600
validation passes; llvmpipe reports an upstream write-after-write warning.
The isolated resource-build benchmark does not establish a clear gain.

AI-assisted: yes.
Prepare binding reflection with each compiled variant, share its access flags across materialized results, and populate only live descriptor and push data. Persist the reflection in versioned shader-cache entries. Add ordering, filtering, lifetime, and cache round-trip coverage.

Written with AI assistance.
Adapt the corrected binding-plan benchmark to immutable descriptor usage metadata.

AI-assisted: yes.
Use the transfer barrier already present in the retained-buffer fixture between two direct RefreshData calls.

AI-assisted: yes.

@adriadam10 adriadam10 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work here. Not mergeable as it stands:

  • This is stacked on #1315, which still conflicts with main and has an open question from @oneandonlydean (no measurable gain in the fixture or in a profiled run, 19 ms vs 21-22 ms per 6,000 builds). This PR needs that answered first.
  • Against current main it conflicts in 13 files (ShaderResources.{hpp,cpp}, DescriptorBindingBuilder.{hpp,cpp}, Recompiler.{hpp,cpp}, ShaderDiskCache.{hpp,cpp}, libSceAgcDriver/CMakeLists.txt and the Graphics/Recorder/ShaderDiskCache/ShaderMemory tests). #639 rewrote buildPrepare/buildComplete with compact bindings and runtime image metadata, so this is a redesign on the new structures rather than a mechanical rebase. The description says the change doesn't replace #639's ABI work; please show what is still left to share after it.
  • Please post before/after numbers on a rebased tree (same fixture or a real title, several runs), including the shader-cache FormatVersion bump, which also collides with other open PRs touching it.

Once #1315 is settled and this is rebased with numbers, happy to look again.

@TibixDev

Copy link
Copy Markdown
Contributor Author

Review-pass update:

Audited today's types: CompiledShaderArtifact already holds compiled/static shader data, while ShaderInvocation holds specialization, live descriptor bindings, push constants and vertex attributes. The remaining candidate is immutable descriptor-access vectors; live values still have to be materialized per invocation. This is narrower than treating the old complete binding object as shareable.

The serialized cache is now FormatVersion = 27; replaying this branch's old version edit would be incorrect. A replacement needs a new version chosen against its landing base and round-trip/invalidation checks. #1315 still has no demonstrated title benefit, and this stacked head has no isolated modern comparison. No code push or claim that the prior 448 → 160-byte layout result establishes runtime benefit on the new structures.

I have updated the PR description to match this status. The PR remains open; the outstanding review requirements are not being marked resolved. AI-assisted audit.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Closing for now: the author marked it blocked pending a redesign after #639. With the queue this large we're closing stalled perf PRs that don't show a gain in a title; reopen it with before/after numbers from a title scene (the summary lines from the performance report form) if you pick it up again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc* build needs rebase Conflicts with main, rebase needed shader core/shader

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants