Repository navigation
upstream PRs: BDA and memory emission in the shader output (U2) - #473
Merged
Merged
Conversation
RADV inlines every SPIR-V function, DontInline included, so every get_bda_pointer call site becomes its own copy of the page-table search, and NIR's copy propagation of variables grows with each copy. The lookup now keeps its search bounds in phis and records its header and range faults from one call site each, picking the first failing check; a byte read and the byte fallback of a span read loop over one lookup instead of unrolling one per byte or dword. (cherry picked from commit 2d3a970) Ported from upstream PR boykopovar#3655 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A formatted load whose V# is selected on the GPU emitted all 77 format cases in its shared function, and drivers that inline every function, like RADV, copied all of them into each call site. The load now reads the format's component count, widths, byte and bit offsets and type from a constant table and runs one path for its four components, with the existing conversion of each (type, width) pair selected at run time. Stores and loads through a prepared V# keep the format switch. (cherry picked from commit 369928d) Ported from upstream PR boykopovar#3656 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
agc_driver_gpu_buffer_format loads the V#'s records and word3 from memory and moves them into SGPRs with v_readfirstlane, so the recompiler sees a GPU-selected descriptor and the load goes through the format table. It checks buffer_load_format x/xy/xyz/xyzw and d16_xyzw on two lanes for formats 1-77, selectors 0/1/4-7 on 1-, 2- and 4-component formats, loads with a component past NUM_RECORDS, and that formats 0 and 78 stop the invocation before its stores. The expected values were captured on the format switch (2d3a970) and are identical with the table. (cherry picked from commit 6aaae03) Ported from upstream PR boykopovar#3656 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every access by guest address calls a lookup that binary-searches the BDA range table: about 11 dependent steps over ~1200 ranges, 34 table loads per access. Since boykopovar#639 every buffer access goes through it. The lookups (read, probe, write and atomic) now keep the last range a search of the invocation resolved in four private variables: guest begin and end, device base and permissions. An access inside that range with the wanted permissions returns base + (address - begin) at once. Anything else (another range, a missing permission, an unmapped, zero-byte or wrapping access) takes the full search with every check and fault it records today, and a successful search refills the cache. The variables only hold what the table says, and no shader writes the table, so the result is the search's own answer. On one title's busiest scene (Windows, discrete GPU), GPU time of the draws 59.5 -> 35 ms per frame. APS5_NO_BDA_LOOKUP_CACHE=1 emits the lookups without the cache. The switch is part of the shader disk cache key. (cherry picked from commit 443583a) Ported from upstream PR boykopovar#2837 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…span read A scalar load of several dwords (s_load_dwordx8) becomes one LoadAddressU32 per dword, and each took its own span read: a probe of the page table and a fallback that looks every byte up. Loads in one block with equal address operands and offsets within 16 bytes of the first now share its span read. Instructions that write memory end a group; the address operands are compared by content, since each load carries its own GetAddressResource and SelectU32 values. A title's main-menu pixel shader goes from 136 span reads to 34. AMD's Windows compiler inlines the read helpers at every call, and did not finish that shader before. (cherry picked from commit 4577633) Ported from upstream PR boykopovar#4931 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An s_buffer_load_dword* whose V# the recompiler reads at run time (a GPU-selected V#, for example one built with v_readfirstlane) went through the vector buffer descriptor checks. A nonzero TYPE in word 3 made it return 0 and record an invalid-descriptor fault, and the base was used with its two low bits, so a base off a dword boundary read shifted bytes. SMEM uses only the base, stride and num_records of the V#, and the two low bits of the base and of the final address are ignored (AMD RDNA2 ISA guide, 30 November 2020, section 7.1 Table 27 SBASE and section 7.2.1 "Reads using Buffer Constant"). The capture-time evaluator already follows both rules, and the bound path already drops the two low bits. For scalar buffer loads the runtime path now masks the base and skips the TYPE check; vector, formatted and atomic accesses keep it. agc_driver_scalar_buffer_gpu_descriptor runs s_buffer_load_dword and s_buffer_load_dwordx2 through such a V# with the base 0 to 3 bytes off a dword and TYPE 0 to 3, in wave32 and wave64. (cherry picked from commit 86fdb52) Ported from upstream PR boykopovar#5161 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…compute modules (cherry picked from commit 573ba3b) Ported from upstream PR boykopovar#5353 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aea7c5c20a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const auto none = state.module.AllocateId(); | ||
| state.module.AddFunction(spv::OpCompositeConstruct, vector, none, zero, zero, zero, zero); | ||
| const auto all = EmitValueOrDefaultIfCondition(state, ActiveArgument(ctx, inst), vector, none, [&]() { | ||
| const auto values = EmitBdaDwordReads(ctx, inst, GuestAddressBase(ctx, inst, mem), mem.offset, 4u, false, used); |
There was a problem hiding this comment.
When separate scalar load instructions use the same base with nearby offsets, AdjacentBdaLoads can group them even if their MemoryFlags::pc values differ, but this call emits every lookup and fallback using only the first instruction's PC. If a later grouped offset is unmapped or otherwise faults, the fault record therefore identifies the earlier instruction, making the reported shader location incorrect. Restrict grouping to members with the same PC or preserve per-member PCs in the grouped fallback.
Useful? React with 👍 / 👎.
This was referenced Oct 15, 2026
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
6 participants
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.
Ports unit U2 of the upstream open-PR batch (BDA lookups and memory emission in the recompiler output). Upstream reports smaller shader code and faster driver compiles on AMD; neither GPU time nor Metal compile time has been measured on Metal yet. boykopovar#3655 and boykopovar#4931 replace unrolled reads with loops, so Metal GPU time needs a measurement before claiming a gain. Every PR is its own commit, and each commit body says "Ported from upstream PR #N".
Refs upstream-pr-batch-1
Ported
APS5_NO_BDA_LOOKUP_CACHEreaches the disk-cache key through the generated recompiler-switch list.s_buffer_loadthrough a GPU-selected V# reads the V# as SMEM does. The upstream debt-doc hunk was dropped because that file is not in the fork.tests/execution/BufferDescriptorRuntime.cppbelongs to upstream registration-form work that is not on main.No ShaderDiskCache FormatVersion or request-serializer bump. None of these PRs change the payload layout, and emitter changes already invalidate cache entries through the cache's source version.
Skipped
EmitBdaByteLoop). The rest of perf(shader): read the slow paths of BDA dword and span reads in loops boykopovar/AnyPS5#4976 would have added a second loop that uses function variables.buffers_u8andbuffers_u16as aliased views of the same fixed-sizebuffersbinding. Metal advertises StorageBuffer8BitAccess, so the views would be emitted, and SPIRV-Cross MSL rejects a second view of a fixed-size descriptor array outside argument buffers ("Attempting to alias same binding with a descriptor array..."). That is the same failure that dropped the upstream byte-store commit in sync 8D. Taking it needs a Metal-side rework, such as merging the views in the bridge or using argument buffers.Evidence
ctest: 100% passed, 0 of 37 failed.agc_formatted_buffer_once_tests(x86_64): passes. A GPU-selected formatted load drops from 114447 to 16821 SPIR-V words, compared with a main build. A known-descriptor load is 1947 words on main and 2638 here.agc_shader_memory_testscannot link on macOS, on main or on this branch, becauseGuestMemory.cppusespthread_getattr_np. Its verify functions were run one at a time with a stub GuestMemory, on this branch and on main. Both gave the same result: 32 passed, includingverifyBdaReadFallbackFunctions, which carries the new fix(shader): keep each BDA page-table walk to one inlined copy per read boykopovar/AnyPS5#3655 and perf(shader): read adjacent BDA dword loads of one block through one span read boykopovar/AnyPS5#4931 checks. Both also failverifyPixelRequestSerialization(it expects serialization version 19) and stop with a bus error atverifyGuardedNullPointers(this needs the real guest-memory fault handling). Neither of those comes from this change.GpuBufferFormat,ScalarBufferGpuDescriptor,BdaExecution) need the Vulkan device path and were not run on macOS.[metal] skippeddraws or dispatches, no shader conversion failures, and a clean guest exit.🤖 Generated with Claude Code
https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj