Repository navigation
metal: GPU-side sync for labels, waits and indirect draws/dispatches (async phase 2) - #517
Conversation
Phase 2 of the async GPU roadmap: the worker no longer drains the timeline for indirect arguments, and label points and waits on a value no longer drain either. - DISPATCH_INDIRECT: the GPU reads the group counts from the guest argument buffer (dispatchThreadgroupsWithIndirectBuffer) instead of the CPU reading them after a drain (cause 0x16). - DRAW_INDIRECT / DRAW_INDEX_INDIRECT and their multi forms with a packet count: the Metal indirect draw reads each record from the guest buffer, with the index and vertex buffers bound over their declared ranges. Where no shader reads the CP's base-vertex or first-instance SGPR write (the Constant rule), a blit copies each record's count, instances and first index next to the driver's constants on the GPU. Fans, strips without restart, mesh, rect lists and barycentric capture keep the CPU path (cause 0x10f). - Label points (RELEASE_MEM / WRITE_DATA behind committed work) are signalled on a MTLSharedEvent the timeline owns (DeferSignal), so GPU work on another queue can wait for them. A label with an end-of-pipe interrupt, or one behind a full batch, commits the batch there, so it is not stored behind work encoded after it. - WAIT_REG_MEM: pending label stores are tracked until they run (not until any completion ran), and a wait on a value no pending store of ours provides polls for it while the timeline retires work, instead of draining first. ANYPS5_METAL_GPU_INDIRECT=0 and ANYPS5_METAL_GPU_WAITS=0 restore the CPU paths. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj
|
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 |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fa70cd0b8
ℹ️ 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".
| if (!GpuWaits()) CompletePriorGpuWorkAndCopyBack(opcode); | ||
| else timeline.Flush(); |
There was a problem hiding this comment.
Drain GPU writers before polling an external wait
When deferred GPU work may write the waited range, merely flushing it lets the first Pm4::WaitSatisfied read stale guest memory before that work completes. If the old value already satisfies the comparison but an earlier draw or dispatch subsequently writes a nonmatching value, the loop exits immediately and later commands run without the required wait. Drain when timeline.MayHaveWritten(address, address + bytes) is true (including unknown writes), or otherwise ensure the producer completes before the initial CPU poll.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 965b426: a WAIT_REG_MEM whose awaited bytes committed GPU work may write (MayHaveWritten, including unknown BDA writes) or a pending label store of ours overlaps now drains first, as before. Only a value no pending work of ours writes is polled without a drain.
A WAIT_REG_MEM on bytes that committed GPU work or a pending label store of ours may still write drains first, as before, so the wait never passes on the value from before that work (Codex review on #517). Only a value no pending work of ours writes is polled without draining. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj
Phase 2 of the async GPU roadmap: GPU-side synchronization instead of CPU drains.
What changes
dispatchThreadgroupsWithIndirectBuffer). Before, the worker drained the timeline and read them on the CPU (cause 0x16).MTLSharedEventthe timeline owns (MetalTimeline::DeferSignal), so GPU work on another queue can wait for them (Phase 4).ANYPS5_METAL_GPU_INDIRECT=0andANYPS5_METAL_GPU_WAITS=0restore the CPU paths.Drains per live-map replay frame (local drain-site log, not committed)
So 10 of the 13 "0x10f" waits in the frame profile come from the replay harness, not the driver. The guest-driven drains left in the frame are the one barycentric indirect draw and the Phase 1 reads.
Correctness
Since #514 the live map has no stable hash. Each repeat's output was dumped and compared:
replay-ref-v2.png, every repeat, for this branch and for an origin/main build of the same base:metal_utility_tests):LabelEvents.mm: a consumer on a second queue, committed before its producer, waits for the label event on the GPU and sees the producer's writes with no CPU drain. A polled label is stored while work encoded after it is not even committed. Also covers a label behind committed work and a label riding its batch.ShaderPipeline.mm: an indirect dispatch runs with group counts earlier GPU work wrote; zero groups dispatch nothing.Timing A/B
10 interleaved pairs against an origin/main build of the same base, live map, under the GPU lock. Traced builds (frame-trace instrumentation, not committed); medians of repeats 2-3 of each run; each variant replays its own warmed clone of the capture.
The medians move with machine load between pairs (each half drifts), so the paired ratios are the comparison: neutral to about 2-5% better. Command buffers per frame stay at main's 136-139.
--maprun: the A/B is neutral, not positive.Codex review findings
MayHaveWritten(including unknown BDA writes) or a pending label store overlaps the awaited bytesFinal commit re-checked, every repeat: live vs v2 70, 80, 92, 77, 70, 117 px (0-3 px outside main's envelope); menu 484 (warm-up), 68, 51, 92, 94, 94 px (0-26 px outside main's envelope, character only).
🤖 Generated with Claude Code
https://claude.ai/code/session_01DgW4n3qYgi5jAAohEmXRQj