Repository navigation
perf(agc): run the dispatches a compute ring leaves unordered side by side - #9176
Celegans12 wants to merge 1 commit into
Conversation
github-actions
Bot
added
perf
|
This pull request is labeled |
|
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.
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.
|
|
Run on AMD hardware. Host: Arch Linux, kernel 7.2.11, AMD Radeon RX 7900 GRE (RADV NAVI31), Mesa 26.2.5, GCC 16.2.1, Release,
|
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
The microbenchmark is a real signal (8 independent dispatches 0.80 ms against 6.3 ms with a barrier between each pair on an RTX 3080 Laptop). The design decides overlap from the absence of packets the game wrote, and that is the wrong evidence.
- "Unordered" here means no packet between the two dispatches other than
SET_SH_REGandNOP. That says the game didn't ask for ordering; it does not say the dispatches are independent. Titles that skip aCS_PARTIAL_FLUSH/ACQUIRE_MEMbetween two dispatches may still depend on the second starting after the first has launched, and nothing in the PR shows that none does. Before this PR the emulator imposed full ordering and hid that; now a dispatch that reads what the previous one wrote through the same memory (not the sameresourcesobject) runs concurrently with it. The PR's own tests only check that dispatches over separate resources keep their results. - The emulator already has the evidence the PR needs: the read and write ranges of each dispatch (
PassHazardsdoes it for draws in one render pass, and the staged-element code knows each element's extent). Overlap a dispatch only when its ranges are disjoint from the run's, and treat unknown accesses (BDA, indirect) as conflicting, the wayPassHazardsdoes. That makes overlap a proven-independent case and costs one range check; the packet-pattern rule makes it a guess. Please add that check, and a test where dispatch B reads what A writes, with onlySET_SH_REGbetween them, and requires B to see A's result. - The only measured effect on a game is in "our own tree (not
main)": overlap off added about 146 ms of summed GPU time per frame (+19%) on PPSA26344, with presents unchanged because the scene is CPU-bound. That is a GPU-time number on a scene where the frame rate doesn't move. Post the summary lines of main and this PR from the same scene, and one GPU-bound scene where the benefit would show in frame time. - NVIDIA only. AMD and Intel hardware run unordered dispatches differently; an AMD run of
agc_driver_dispatch_overlapand a game scene would show it's not specific to the NVIDIA barrier cost.
CnRJay
left a comment
CnRJay
left a comment
There was a problem hiding this comment.
A data point for the hazard question in my earlier review: #9287, stacked on this PR, says "PPSA26344 v1.008: renders correctly in our tree with this change (geometry broke before the guards)" and adds a rule that "a dispatch does not join when one of its ranges overlaps a range of the run" when one side is a staged copy. So overlapping dispatches already produced wrong geometry on this title once the copies were allowed inside a run.
That shows the dispatch overlap in this PR needs the same guard in the base case: a dispatch must not join an open run when its read or write ranges overlap the ranges of the run (reads against writes, writes against writes), and unknown accesses (BDA, indirect) must conflict, not only the staged-copy case #9287 covers. Please add that check here, with a test where dispatch B reads what A wrote with only SET_SH_REG between them, rather than leaving it to a follow-up that depends on this one landing first.
0c93069 to
49625ed
Compare
|
Thanks, you're right that the packet pattern was the wrong evidence. No CS_PARTIAL_FLUSH between two dispatches only says the game didn't ask for ordering, not that the dispatches are independent, and #9287's geometry history shows what happens when that gap is left open. I've amended the commit so a dispatch joins an open run only when its accesses are disjoint from the run's: every bound buffer range counts as a read, every written range as a write, and storage images (including sampled views of them) are compared by image. Reads against writes and writes against writes conflict. An address-based or BDA build counts as writing all memory, and indirect, LDS, thread-dimension and staged dispatches never join.
You're also right that the main-vs-PR summary lines from one scene and a GPU-bound scene are still owed. In the part of Yotei's world we can reach we're CPU-bound in the committer, so frame time won't move there; I'll post both summary lines and look for a GPU-bound spot. We don't have AMD hardware either. Could someone with an AMD card run |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Review
I reviewed the dispatch eligibility, access-range conflict checks, deferred barrier lifecycle, and the added ordering/hazard cases. I did not confirm a behavioral defect in the read/write conflict predicate from static inspection. I’m requesting changes because the branch is far behind current main, the performance report does not yet meet the repository’s evidence rule, and some active overlaps need to be recorded and coordinated.
Bring the branch and validation current
This head is 721 commits behind current main (merge base de4ef842b5). The tests in the description were run against that old base. Please rebase on current main, resolve any conflicts, and rerun the focused dispatch tests on both main and this head.
This changes the AGC driver, so the PR checklist also requires the focused tests on both revisions with VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, ANYPS5_NO_SHADER_CACHE=1, and ANYPS5_REQUIRE_VULKAN=1 under ctest -V, including a comparison of new VUID- messages and confirmation the requested validation layer loaded. The reported Windows/WSL test counts and dispatch cases are useful, but the description does not report this comparison.
Performance report
The repo requires a perf PR to provide valid summary lines for main and the change from the same machine and scene, with traces. The current perf numbers missing label reflects the gap: the PPSA26344 result compares the feature enabled/disabled in “our own tree (not main),” and the 8-dispatch microbenchmark is not a main-versus-change scene report.
Please add same-scene, same-machine main and rebased-head summary lines and traces as described by the performance issue form. Keep the enabled/disabled run as a useful feature-isolation result, but label it separately from the required main comparison.
Open overlaps
The generated overlap report lists currently open #6946, #9508, and #7449 in code changed here; the PR body currently records #7920 and mentions #8278/#8992. In particular, #6946 changes packet execution ordering in the same function, #9508 is stacked on #6946, and #7449 touches the dispatch recording/barrier path. Please update the overlap notes, agree on integration order with the affected authors, and add a Depends on entry if this branch actually depends on another PR. The #7920 barrier interaction is already called out; please preserve that coordination as well.
Checks
git diff --check: passed.tools/check_conventions.pyagainst currentmain: passed.- Current CI: eligibility, conventions, labels, and report passed; Linux, Windows, register-context, relinker, ASan relinker, and write-watch jobs were skipped.
- I did not produce a local build/runtime result for this head in this session. The runtime results in the PR description are author-reported and are from the older base above.
The test matrix and explicit APS5_NO_DISPATCH_OVERLAP control are useful. I’ll revisit after the current-base validation, performance comparison, and overlap notes are updated.
…_NO_OVERLAP_RANGE_CHECK=1 off; an APS5_AB switch) Port of boykopovar#9176's range check: a dispatch joins an open run only when its buffer ranges and storage images are disjoint from the run's (reads against writes, writes against writes); address-based/BDA builds and bindless image dispatches count as accessing everything. [overlap-range] counts refusals, and conflicts that joined while the switch was set. AI-assisted: yes
… side Every dispatch records a barrier before and after itself, and on NVIDIA any barrier between two dispatches drains the GPU. A dispatch of a compute ring (queue != 0) with only SET_SH_REG and plain NOPs between it and the ring's previous dispatch in the same submission may join that dispatch's run: it records no leading barrier, and the run's trailing barrier is deferred until any other command records it. It joins only when its buffer ranges and storage images are disjoint from the run's (reads against writes, writes against writes); an address-based or BDA build counts as accessing everything, and a run never takes the same resources object twice. APS5_NO_DISPATCH_OVERLAP=1 turns it off. AI-assisted: yes
49625ed to
baae58f
Compare
|
Thanks for the careful pass, especially the reminder that the validation-layer comparison is part of the checklist. I hadn't run it. I've rebased onto current Validation: I ran On performance, I've split the two results. The game number stays as a feature on/off result from our own tree, and it's labelled that way. A On overlaps, @AlxcBTW @hugodearaujo @Raghav314, I don't think there's an ordering dependency in either direction. Whoever lands second adds one condition:
If any of you would rather this one went first or last, tell me and I'll rebase to match. Details
|
|
@Celegans12 no preference on order from #7920's side, and your condition is the right one. With So a dispatch that writes markers should stay out of runs, as the NV checkpoint does: |
This branch has not been deployed
5 participants
What
Every dispatch records a barrier before and after itself, so dispatches the game leaves unordered never overlap on the host GPU, and on NVIDIA each barrier drains the GPU.
Recorder::Commands, which closes the run first.APS5_NO_DISPATCH_OVERLAP=1records every dispatch as before.Overlaps (no ordering dependency; whichever lands second adds the condition):
!mirrorClearedinoverlapCapable); textual conflict in the leading barrier.ShaderResources::RunAccesses, or count as unknown.Tested
maincc00785: ctest 1886/1892 (the 6 failures fail onmaintoo:log_macrosx2, threeagc_driver_sample_order,guest_kqueue). Linux (WSL, lavapipe) job on the merge: 1823/1843, the same 20 fail onmain.VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest -V -R dispatch):main12/12, this head 14/14; the loader shows the layer added, no "was not found", noVUID-on either.agc_driver_dispatch_overlap: four unordered builds share one run (3 join); with CS partial flushes none joins; RAW, WAR and WAW pairs with only SET_SH_REG between them don't join and get the ordered result; a disjoint pair joins;_offexpects none. Without the range check the RAW case fails "1 joined a run, expected 0".main12.5 ms, this PR 1.79 ms; with CS partial flushes 12.3 vs 12.8 ms.main): PPSA26344 overlap off added ~146 ms GPU per frame (+19 %), presents unchanged (CPU-bound). Nomain-vs-change game scene yet: plainmainstops on Yōtei's loading screen (~160 s) and later aborts on a 10_11_11 blend (feat(agc): blend into 11_11_10 float color targets in the pixel shader #9020 follow-up in progress).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)