Repository navigation
test(agc): support draw profiling in the graphics fixture - #11566
oneandonlydean merged 4 commits into
Conversation
AI-assisted: yes
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 1e38ada86ffa741c36f817c1c92102875fb66f22; no actionable findings. The mock query-pool implementation bounds-checks reset/read indices, returns VK_NOT_READY until all requested timestamp queries are populated, and writes 64-bit results using the requested stride. The new test registration opts into profiling and disables the shader cache; the report correctly distinguishes mocked lifecycle coverage from real GPU performance evidence. The author reports the focused Windows GPU and validation-layer runs passed with no VUIDs; hosted eligibility, conventions, intake, labels and relinker checks pass, while unrelated product jobs are skipped. I did not rerun the Windows GPU test locally.
|
Correction to my review summary: the hosted relinker and ASan jobs were skipped, not passed. Eligibility, conventions, intake, labels and progress-report checks passed; product jobs were also skipped. My approval is based on the diff review and the author-reported Windows focused run, not hosted relinker results. |
|
Additional local verification at unchanged head
The hosted relinker and ASan jobs remain skipped; these local results do not change that status. No source change, game-performance claim or new push accompanies this update. AI-assisted: Codex (Luna execution, independent evidence review). |
|
Manual follow-up to the overlap warning in #issuecomment-6427054960, for this PR at I had posted test results without completing this requested review. I have now inspected the complete patches for both shared files in all 66 listed open PRs. Candidate heads were rechecked after review and were unchanged.
Exact reviewed candidate heads and dispositions:
AI-assisted: Codex (manual comparison by Luna and independent review, consolidated evidence check). Bounded October29 new-overlap supplementThis supplements the historical66-row manual review. Bot6427054960 is frozen at2026-10-29 22:28:02Z:74 current rows equal66 historical IDs minus17 removed plus25 added. Removed rows remain historical; changed retained heads have not all been refreshed. This review concerns the existing #11566 timestamp mock/profile fixture contribution, without retargeting old approval or test evidence. I read complete listed shared-file patches for the25 new candidates below. #12862 omitted its large CMake API patch; I reconstructed it from immutable merge-base
Concrete integration findings:
This is complete new-row shared-file source review, not a clean pairwise merge, combined build/test, author-order agreement or all-current-rows clearance. Retained-head drift and pre-existing manual obligations remain pending. Marc's approval body still names historical AI-assisted: yes (Codex). Bounded later bot-row updateThe bot's later snapshot at 2026-10-29 22:59:53Z lists 70 candidates. Compared with the previously reviewed 22:28 snapshot, #12702 entered and #9533, #9717, #9742, #12567 and #12568 left. Compared with the historical 66-ID table, the current table has 21 newly added rows and 17 removed historical rows. The earlier 25-row source supplement remains bounded to its reviewed heads; a row leaving the warning does not erase that review or prove integration. The complete shared CMake patch for #12702 at The refreshed #12904 head is This is bounded shared-file source review against #11566 AI-assisted: yes (Codex). October 30 — additional skipped shared-source reviewThis addendum pins #11566 at Here Graphics and fixture scopes
Nine build-file scopesNine additional CMake-only scopes, pinned against #11566 Keep all four #11566 selections: default
No source/cache/build/ref/public changes or new tests were made. No new implementation prerequisite or author agreement is inferred from these CMake changes. Actual future compositions still require their own source/build/runtime review where relevant. #10492/#10497’s duplicate registration names are a concrete merge-preservation detail, not an invented order requirement. #10043’s generated shader/library sources must remain in the build graph; their external implementation/dependency compliance was not reviewed here. #10846 receives no clearance for codecs, shader/resource semantics or any other path from the unrelated #12250 work. No claim is made about all84 rows. The removed #12904 entry is not evidence of landing or compatibility. This addendum does not refresh every retained candidate head or clear the whole 84-row table. It records shared-source preservation requirements only: no combined build, GPU/runtime, performance, console, author-order or implementation-dependency approval is inferred. Existing #11566 runtime results retain their tested commits and environments. AI-assisted: yes (Codex). Five moved-head shared-source follow-upsFollowing the twelve-row supplement, I checked the five moved heads identified in the retained-row metadata pass against #11566
These five prior reviews were originally pinned to target This source review identifies concrete preservation work for a later composition. It does not establish a clean pairwise merge, combined build, mock/real-GPU runtime, performance, console result, production correctness, author agreement or implementation dependency/order. Existing CI, review and coordination obligations stay separate. AI-assisted: yes (Codex). Bounded graphics shared-source follow-upOwned #11566 remains
For9020/9335, the four historical per-path added/deleted sequences match, but old accepted heads5f728740/976fc9c3, raw files and patch context differ. This pass checks preservation against the current owned fixture; it does not advance old runtime or human approval. The new6501 registration has a separate environment. A single end metadata check found the four candidate heads and both owned heads unchanged/open. Actual selected patches were reconstructed from immutable merge-base/head source; no unshared paths were reviewed. AI-assisted: yes (Codex). Current skipped-file follow-through — #11566
|
| Candidate/head | Shared scope and preservation finding |
|---|---|
#7395 4769d5a7456b |
CMakeLists.txt: Seven nested image/sampler and pointer-phi sampler/ballot/copies cases. Preserve exact add_test names/arguments once each, cache-off and no-pipeline-journal environment, skip77, timeout30, source-root working directory and Windows libc PATH on the separate image-table executable. A future multi-PR composition must deduplicate shared names and retain the union; these cases do not replace our graphics profile selector. |
#9852 470cbe5b01a3 |
CMakeLists.txt: Six additions register agc_driver_atomic_passes_split on existing agc_driver_atomic_passes_tests. Keep APS5_NO_ATOMIC_PASSES=1, skip77, timeout60 and Windows libc PATH on this distinct split case. Fresh470c review; older e13 finding is not reused and old/new patch identity is not asserted. |
#10846 0a2cf6402870 |
CMakeLists.txt: Retains exact prior public current-target source finding:33 additions create spanned-wave policy/guard target, eleven image-table/refusal cases and guarded SPIRV-Tools linkage. Reuse same-head/path finding, not new approval. Preserve target includes/shader_recompiler/libc links/unwind/testing participation, guarded compile definition and SPIRV-Tools-static link, normal table cache/journal/workdir/skip77/30s/PATH, and refusal-only APS5_SPANNED_WAVE=yes plus cache-off. |
Preserve our agc_graphics_profile_draw registration on the no-argument agc_driver_graphics_tests, its cache-off/profile environment, skip77, timeout60, source-root working directory, Windows libc PATH, command-batch-fence-failure selector and query/timestamp mock bounds and lifetime checks. Separate image-table and atomic-pass cases must not replace that profile selector. No combined build/test, GPU validation, foreign implementation acceptance, order, latest-main or whole-table clearance follows. Other current rows and compared-overlap obligations remain open.
October 30 — six additional current shared-file rows
This bounded source review pins #11566 at c6cbfa6 and uses the exact current candidate heads below. The updated overlap table contains 73 distinct PR IDs. The existing #7844 row remains covered at its unchanged current head 89d6a7c; this addendum covers only six current rows absent from the prior manual report.
| Candidate / current head | Shared path change | Source disposition |
|---|---|---|
| #9289 / 7cc0194 | CMakeLists.txt: overlap_staged_copies_inline and dispatch_overlap_off test registrations | Distinct names and targets; no duplicate with agc_graphics_profile_draw. |
| #9891 / 80d3a0b | CMakeLists.txt: decoder_png link for agc_driver_gcn_display_present_tests | Separate target-link statement. |
| #9895 / 853389c | tests/Graphics.cpp: GcnDepthTileModeTests and its main call | Separate from the timestamp-query mock additions. |
| #11104 / c3a1821 | CMakeLists.txt: adds a target path argument to agc_driver_unsupported_diagnostics | Separate existing test command. |
| #12536 / aa5cb52 | CMakeLists.txt: adds agc_frame_limit executable and CTest entry | Separate target and test name. |
| #13148 / ac75125 | CMakeLists.txt: adds LiveAllocations.cpp to agcDriverSources and agc_live_allocations test target | Separate source-list and test-target blocks. |
The target profile CTest remains a distinct no-argument agc_driver_graphics_tests invocation with its own profiling environment and properties. Its Graphics.cpp patch adds timestamp query-pool mock support; none of these six candidate patches edits those mock hunks. Preserve both sides’ additions. This is only a source-level disposition of these six rows: it does not establish clean branch composition, runtime behavior, test results, author order, dependency clearance or review acceptance. It does not clear the rest of the current overlap table.
AI-assisted: yes (Codex).
Selected CMake source follow-through — frozen #11566 c6cbfa693601
Reviewed six newly selected CMake relationships from overlap comment6427054960 (updated2026-10-31 15:58:30UTC), against the exact target CMake blob 6b9abdceef980f11e458245f61154fce555e72d2. This is a dated source scope, not a refreshed clearance of the67-PR/74-path table.
| PR / frozen head | Selected-source disposition |
|---|---|
#7508 5e9d5c26b9243096fd49f380f39434965fa83d8d |
Adds a separate agc_relocation_runs executable/test linked to libc with CXX extensions off, unwind setup, BUILD_TESTING inclusion and Windows libc PATH. Preserve this independent target alongside the unchanged graphics-profile registration; corresponding foreign fixture/runtime is outside this review. No new timeout or skip policy is supplied by this hunk. |
#9248 c3060572f3f2e5a2e77b6652c26d192b9595ad0e |
Adds three variants of the separate draw-cache-reuse executable: indirect, push-word, and push-word verification. Retain their cache-off/indirect/profile flags, push-word and verify flags, skip77, expanded FATAL fail-regex list, Windows libc PATH and pre-existing APPEND recipe/register-key settings. Their APS5_PROFILE_DRAW setting does not replace our distinct agc_graphics_profile_draw registration or prove equivalent workload/timing. No explicit timeout is added for the three variants. |
#10940 a273ee4cd0d0811f259d72f40cd6139fc40cea80 |
Adds the existing two-device pending-store executable with --label; preserve ANYPS5_NO_SHADER_CACHE=1, APS5_STORE_AT_LABEL=1, skip77, timeout30, source-root working directory and Windows libc PATH. It is a separate selector from the graphics profiling target; matching foreign --label implementation remains coupled and unqualified. |
#12746 275819376d4644f1b8238ad043de5e8582e55ee4 |
Adds four texture-downscale selectors: --levels 2, --levels 3, --refuse-levels and --refuse-minimum. Preserve dynamic arguments, cache-off env, skip77, timeout30, source-root working directory and Windows libc PATH. Keep neighboring runtime-formatted-store timeout and vertex/export selectors; matching foreign downscale implementation remains coupled and unqualified. |
#10492 d6589d36070bb4d2f60949499a873fab2bba769b |
Exact accepted topic hunk preserved apart from line offsets: retain three native-depth publication selectors and their dispatch/draw-pipeline APPEND env, timeout30, cache-off/skip77/source-root working directory and Windows libc PATH, including retirement recipient. |
#10497 99e4666c92d1dcdb7f07307a277851c06de801c6 |
Exact accepted topic hunk preserved apart from line offsets: retain the same three publication selectors plus attachment/sampled-buffer-feedback selectors and their properties/Windows libc PATH. If composing10492 and10497, deduplicate their three identical shared test registrations rather than add them twice; no author-order agreement inferred. |
Each reviewed topic patch reconstructs its full captured base/head CMake source, and its old hunk context matches the owned target uniquely. Independent in-memory hunk application preserves the exact agc_graphics_profile_draw command, cache/profile environment, skip77, timeout60, working directory and Windows libc PATH. For10492/10497, the accepted topic hunks match byte-for-byte except line offsets at the newly captured heads; inherited later-main CMake changes are outside that reuse.
No configure, build, test or Git merge was run. These findings do not qualify foreign implementations, full-branch compositions, matching runtime behavior, combined profiling workload/timing, author order, dependencies or the remaining table. Removed bot rows remain historical findings; removal does not establish merging or no overlap.
|
@MarcZX10 Could you recheck current head Both focused builds passed. Four cases per arm passed: mock graphics/default and profile plumbing, plus two real Vulkan recorder timing cases. The real cases record the same RTX 4080 SUPER and actual Khronos instance/device insertion, zero skips/VUIDs/missing layers, and matching disclosed loader diagnostics. The PR body retains the exact current qualification and historical evidence; the manual overlap comment separately pins reviewed scopes and remaining drift/order limits. Your earlier review text names AI-assisted: yes (Codex). |
Re-checked — same approved headThe current head is still |
✅ NVIDIA GPU test: passedPR head
Unstable on main (3, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (3, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (5 IDs in 421 tests)Skipped (42)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
Re-checked — GPU report reviewedThe exact-head NVIDIA run reports no new failures or validation IDs in the compared tests, while The report covers head |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
I reviewed head 9d7892e7f91aca9a29790f401bd7638e77e9d01f against base 613f9b8edf056318b14ff96fe3aceda59a419323; the diff remains limited to the graphics fixture and its CMake test registration, and I confirmed the fragment is included by the directory's CONFIGURE_DEPENDS glob. Local git diff --check and python3 tools/check_conventions.py --base 613f9b8edf056318b14ff96fe3aceda59a419323 passed. Hosted Windows, eligibility, conventions, library portability, labels, and report checks pass; Linux and specialized jobs are skipped. The author reports the exact-head focused Release fixture and GPU recorder tests passed; I did not independently rerun them. I did not build or run CTest locally. I found no blocking issue and approve this head.
✅ NVIDIA GPU test: passedPR head
Failing on main too (4, not counted against this PR)Validation IDs already present on main (5 IDs in 341 tests)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
What
Extend the mocked Vulkan timestamp-query lifecycle in the AGC graphics test fixture and register
agc_graphics_profile_drawso the fixture exercises draw profiling.Tested
Current-main integration verification: merge commit
c6cbfa693601609cb2209fda8b1b5b1fc65c56d4(tree5e019e0213794591df8d0e2869aaf7dfd5087796) preserves published ancestry and merges frozenorigin/mainfdd1753204a4732e1011539cc9d3bb69f70b27a6. The contribution against that main remains only the two test-related files, with 45 additions and 2 deletions. Candidate and baseline production source both use that exact main commit and recursive submodule pins. For a fair paired fixture check, the baseline received a byte-identical test-only overlay of the same two files (SHA-256 values recorded); this does not create a production-code difference or a negative-control result.Both focused Release builds passed with WinLibs GCC 15.2.0, the same Release/testing options and FFmpeg dependency pin. The baseline and candidate each built
agc_driver_graphics_testsandagc_driver_recorder_testswith parallelism 2; no install-rule skip was used.agc_driver_graphicspasses 1/1 (8.712s) andagc_graphics_profile_drawpasses 1/1 (8.781s). These run the enlarged graphics fixture with mocked Vulkan, respectively without and withAPS5_PROFILE_DRAW=1; they exercise query-pool lifecycle and profiling plumbing.agc_driver_graphicspasses 1/1 (9.496s) andagc_graphics_profile_drawpasses 1/1 (9.401s). This is a paired fixture compatibility check on main production source with the test-only overlay, not evidence that unmodified main registers the profile test.agc_gpu_timing_completeandagc_gpu_timing_incomplete. All six invocations ran serially under one exclusive GPU-lock acquisition; no test was skipped, and every individual test ran under 30 seconds.ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1,APS5_NO_PIPELINE_JOURNAL=1,VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, the same pinned local validation-layer package and verbose CTest. Each real recorder arm shows two Khronos instance insertions and two device insertions; neither log has aVUID-message or the requested-layer-missing diagnostic.VK_INSTANCE_LAYERSadds Khronos validation and two informational lookups where the installed AMD implicit layer lacksvkGetDeviceProcAddr. Mock fixture runs are not used as evidence of real Vulkan layer insertion.Historical published-head b1d/319621 qualification (retained verbatim):
Current-main integration verification: commit
b1d83958a3e225477f61fc2ac812a6eb71923398(verified source treebdfba15fe64927bf7c3ea8c35a062c166f9f8509), preserving published ancestry and merging pinned main319621ba2e578bd053b0baba2877fe70520e0737. Both focused Release builds passed with WinLibs GCC 15.2.0, using the same compiler, Release/testing options and FFmpeg dependency pin. The contribution against that main remains only the two test-related files, with 45 additions and 2 deletions.agc_driver_graphicsandagc_graphics_profile_draweach pass 1/1. These run the full enlarged graphics fixture with mocked Vulkan, respectively without and withAPS5_PROFILE_DRAW=1; they verify mock query lifecycle and profiling plumbing. Separately, real-deviceagc_gpu_timing_completeandagc_gpu_timing_incompletepass 2/2.agc_driver_graphicspasses 1/1, and the two real-device recorder timing checks pass 2/2. Running main's default graphics fixture withAPS5_PROFILE_DRAW=1fails with the exact control diagnosticAGC graphics: missing Vulkan function: vkCreateQueryPool(CTest exit 8). Main has noagc_graphics_profile_drawregistration; the control selectsagc_driver_graphicsexplicitly.ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1,APS5_NO_PIPELINE_JOURNAL=1,VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, the same local validation-layer package and verbose CTest. Each real recorder arm proves two actual Khronos instance insertions and two device insertions, with zeroVUID-messages, no missing requested validation layer and zero skips. The mock fixture runs do not establish actual validation-layer insertion. Both recorder arms retain the same four environment-layer warnings and two implicit AMD-layervkGetDeviceProcAddrlookup diagnostics.Earlier published-head evidence (historical pins and qualifications retained):
Windows 11 Pro 10.0.26300; MSYS2 UCRT64 GCC; NVIDIA GeForce RTX 4080 SUPER, driver 617.42. The retained baseline receipt is b9c30ee; its build and both GPU timing tests passed. Final candidate 1e38ada is based on main da1d222. The only intervening AGC-file change is PCH wiring for execution-test targets, outside these focused graphics/recorder targets, so the baseline was not rebuilt. Baseline
agc_driver_graphicspassed; withAPS5_PROFILE_DRAW=1, it failed as expected on missing mockvkCreateQueryPool. On the candidate,agc_graphics_profile_draw,agc_driver_graphics,agc_gpu_timing_complete, andagc_gpu_timing_incompletepassed. The Vulkan validation layer loaded fromVkLayer_khronos_validation.dll; validation reported 0 VUIDs and no requested-layer-missing warning. Builds used CMake'sCMAKE_SKIP_INSTALL_RULES=ONto skip unused install/export generation without changing compile, runtime, ABI, or unwind settings. Mock timestamps verify query lifecycle and profiling plumbing only; they are not real GPU performance evidence.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)2026-10-31 test-registration split follow-through
Candidate
9d7892e7f91aca9a29790f401bd7638e77e9d01f(tree27573dbc13fa89a250bee1c2690baa9d753e7a1a) is an ordinary merge of publishedc6cbfa693601609cb2209fda8b1b5b1fc65c56d4with frozen main613f9b8edf056318b14ff96fe3aceda59a419323, which contains the landed #12862 registration split. The only resolution moves this PR's profile-test registration and properties intotests/cmake/agc_driver_graphics_tests.cmake; the mock timestamp source is retained. Against that main, the contribution remains two test-related files, 45 additions and 2 deletions. This is a frozen-main qualification, not a claim that the remote tip cannot advance.Windows Release/Ninja with WinLibs GCC 15.2.0 r7: both focused graphics-target builds passed, with at most two compiler jobs. The same cache/options, recursive dependency pins and existing FFmpeg package at pin
9ac4cfd195f1were used for both arms. Generated CTest metadata preserves all 3,200 baseline registrations and adds exactly oneagc_graphics_profile_draw, including its profiling/no-shader-cache environment, skip code 77, 60-second timeout, source working directory and Windows runtime-library path.agc_driver_graphicspassed 1/1 (14.31s). WithAPS5_PROFILE_DRAW=1, that same registered test failed with the specific control diagnosticAGC graphics: missing Vulkan function: vkCreateQueryPool(CTest exit 8, 0.94s). Main does not register the new profile test.agc_driver_graphicspassed 1/1 (13.69s), andagc_graphics_profile_drawpassed 1/1 (14.13s). All four executions were serial, with no skips; these are mocked Vulkan fixture checks, not real-device timing or performance measurements.The preceding real-recorder/GPU results remain historical at their stated source/binary pins, including
c6cbfaversusfdd175; they are not retargeted to this candidate. No new GPU, title, Linux, full-suite, hosted-CI or upstream-landing result is claimed. Compiler warnings are retained, not described as resolved. The initial configure's FFmpeg certificate-download failure was retained; configuration used the repository-supported existing same-pin package instead of disabling TLS verification. No source workaround or dependency change was made.