Repository navigation
fix(agc): copy a linked pipeline's color sample counts only when it has color attachments - #7985
Conversation
|
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.
|
|
Put the counts on the graphics create info instead: The copy-out and the by-value capture that keeps the array alive across the deferred relink are right — only the chain placement needs to move. AI-assisted: yes. |
|
@ebosido444-ai thanks, you're right. AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
Two issues block approval:
-
Please handle a zero-color-attachment pipeline safely. Vulkan permits dynamic rendering with
colorAttachmentCount == 0; a depth-only pipeline may still carryVkAttachmentSampleCountInfoNV, with no color-sample array. The currentsampleCounts.emplaceevaluatescounts.pColorAttachmentSamples + counts.colorAttachmentCountand constructs a vector from that range. If the valid zero-count pointer is null, this forms a null-pointer range. Guard the copy oncolorAttachmentCount != 0and cover or otherwise verify the zero-color case. The struct is also valid in theVkGraphicsPipelineCreateInfochain: https://docs.vulkan.org/refpages/latest/refpages/source/VkAttachmentSampleCountInfoAMD.html -
Please coordinate the
Depends onorder with the authors of open #9508 and #5925. The current overlap scan reports direct same-line edits with #9508 inPipelineLibrary.cpp(link creation and the deferred relink capture). Manual comparison also shows #5925 changes the same link-creation setup and relink lambda. #7642's edits in this file are confined to the split-layout handling and appear disjoint. The checklist currently saysDepends on: none; after the authors agree which changes land first, update it to match the integration order. This follows the repository's overlap rule inCONTRIBUTING.md.
The current hosted eligibility, conventions, labels, and report checks pass. The focused validation results are author-reported; I did not run a local build or GPU test.
|
@Raghav314 @adriadam10, @MarcZX10 pointed out that this PR edits the same lines in This change is small. It finds the I'm happy to go either way:
Is there an order you'd prefer? AI-assisted: yes (Claude Code) |
|
Thanks @MarcZX10.
AI-assisted: yes (Claude Code) |
|
@milescoviello your first two commits here ( Only |
|
@oneandonlydean thanks, done: the branch is now just that commit, ff61956 on current AI-assisted: yes (Claude Code) |
e9c9857 to
ff61956
Compare
|
@oneandonlydean pushed: the branch is now just ff61956 on AI-assisted: yes (Claude Code) |
ff61956 to
6346c5f
Compare
|
@MarcZX10 rebased onto current
AI-assisted: yes (Claude Code) |
|
AMD coverage for the focused set.
|
|
@ycbuzi thank you for the RDNA2 run, the first AMD coverage this PR has. I've added it to Tested and the GPU to the checklist. AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
The zero-color guard addresses the null-range issue, and the current one-commit diff is limited to copying color sample counts only when colorAttachmentCount is nonzero. The author reports Linux NVIDIA validation tests and Windows AMD focused tests passing against corresponding main baselines; no new validation IDs were introduced. Hosted policy checks pass, while product jobs are skipped. I did not run a local GPU test.
The current thread still describes overlap with open #5925 and #9508 in PipelineLibrary.cpp; the checklist says Depends on: none, and I found no agreed landing order from their authors. Please confirm whether the exact current heads still overlap this guard. If they do, coordinate which change lands first and record the order in the relevant checklists; if they no longer overlap, update the description/overlap record. I cannot complete approval until the repository's overlap-order rule is resolved.
|
@MarcZX10 I checked the exact current heads:
Neither PR overlaps these lines now, so I've updated the overlap note in the description and left AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — changes requested for current-main rerun
Your exact-head comparisons resolve the earlier overlap question: #9508 merges cleanly, and #5925's changes are separate from this guard. The remaining blocker is that this branch is still based on cf528e78c6, while current main is 3a2b91981651569163768854a1fcdc7193b6667b.
What I ran
- Reviewed exact head
6346c5f2fefef8eed6745f5cf4399d868876d50fand inspected the overlap update. - Compared #7985 with current #9508 and #5925 heads, checked the merge preview onto current
main, and inspected changes toPipelineLibrary.cppsince the branch base. - Ran
git diff --checkand the current conventions checker against this head.
Results
| Check | Result |
|---|---|
| Current-main merge preview | Raw outputmerge-tree-exit=0; tree e48d0cae9d46132348bbb808db1538f2a48553fa |
| Overlap update | |
| Whitespace check | Raw outputgit diff --check upstream/main...refs/review/pr-7985 — no output; exit status 0 |
| Conventions | Raw outputAll conventions checks passed. |
| Author-reported GPU validation | Reported resultThe PR body reports the focused pipeline/sample-count tests on NVIDIA and AMD with validation on the prior basecf528e78c6; these were not independently rerun here. |
| Branch freshness | Raw valuesMerge basecf528e78c6c8fd30696ccfca6c097f9828be2cf6; current main is 4,703 commits ahead. Current main also changes the optimization key and pipeline flags in PipelineLibrary.cpp. |
Negative control
The author reports that a depth-only pipeline with a null color-sample pointer previously triggered the invalid range construction, and that the guard fixes it. I did not independently run the depth-only Vulkan case on this head.
Findings
Blocking
CONTRIBUTING.mdrequires keeping the branch up to date withmain. Because currentmainhas since changedPipelineLibrary.cpp, the earlier focused comparison oncf528e78c6does not cover the current integration. Please rebase and rerun the same focused tests on currentmainand the updated PR with the required validation-layer settings.
Non-blocking
- The updated overlap note is adequate for the guard: #9508 is compatible, and #5925's stale-base conflicts are outside the guarded sample-count range. Keep the explanation in the PR description.
Sources
- PR head:
6346c5f2fefef8eed6745f5cf4399d868876d50f. - Current
main:3a2b91981651569163768854a1fcdc7193b6667b. CONTRIBUTING.mdat currentmain.
Not verified
I did not build or run Vulkan tests locally. The focused GPU results and negative control are author-reported on the older base. Hosted policy checks pass; full product and specialized hosted jobs are skipped.
To get this to a good state
Rebase on current main, run the focused depth-only/pipeline tests on the same GPU on main and the PR with validation enabled, and report the exact revisions and VUID comparison.
|
@MarcZX10 thanks. Rebased onto AI-assisted: yes (Claude Code) |
6346c5f to
e6d81fb
Compare
|
@MarcZX10 rebased and rerun: head
AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — changes requested
The focused GPU and validation-layer report is detailed, including the same 85/86 result on base and candidate. However, the tested base is main 3a2b919816, while current main is 52c09d567d2fa453494dd720fcf53a98149468e4; this head is 36 commits behind. The contribution checklist requires the branch to be based on current main.
What I ran
- Reviewed exact head
e6d81fbe9bf70441ed4fc17b5b2c4848c8e9298aagainst currentmain. - Ran
git merge-tree --write-treeandgit diff --check. - Checked the author’s exact-head GPU/validation report and current hosted checks.
Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean; current main is not in the branch history |
git diff --check |
Passed |
| Focused GPU tests | Author reports 85/86 on the old base and head in three runs, with the same known TLS failure |
| Validation layer | Author reports matching results and no VUID- messages on that old base/head pair |
| Current-main run | Not reported |
Please refresh onto current main and rerun the focused test and validation comparison there. I did not run a local GPU build, so I cannot treat the earlier base/head comparison as current integration evidence.
…as color attachments Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e6d81fb to
5d1e214
Compare
|
@MarcZX10 thanks. Refreshed onto
AI-assisted: yes (Claude Code) |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed — approved
I reviewed exact head 5d1e214ce3de525b07e5328cb87993fc91d46439 against current main 2960e9a6e7e1fc99c4ed180c71495b5eb83464c7.
What I ran
- Inspected the sole production change in
PipelineLibrary.cpp: it avoids forming a vector range frompColorAttachmentSampleswhencolorAttachmentCountis zero, and retains the existing values when the count is nonzero. - Ran
git merge-tree --write-tree refs/remotes/upstream/main refs/review/upstream-7985; it returned clean tree8e92f48dd104208d3fc7de172916833f5a6a510b. - Ran
git diff --check refs/remotes/upstream/main...refs/review/upstream-7985; exit 0, no output. - Confirmed current
mainhas no later change toPipelineLibrary.cppsince the author's tested base52c09d567d. - I did not run a local build or GPU tests.
Results
| Check | Result |
|---|---|
| Current-main merge preview | Clean |
| Diff check | Pass |
| Focused GPU/validation comparison | Author reports the same 85/86 result on base and head over three runs; agc_driver_pipeline_journal fails identically on both due to the NVIDIA static-TLS limit |
| Validation layer | Author reports it checked 104 devices on each side with no missing-layer or VUID- messages |
| Hosted checks | Windows, conventions, eligibility, labels and report pass; Linux and specialized jobs are skipped |
Negative control
No dedicated control for the zero-color-attachment input is reported. The author-reported base/head run is a regression sweep, not a targeted assertion for that input. I did not run a local control.
Findings
Blocking
None. The guard is limited to the zero-count case and avoids constructing the color-sample range from a null pointer; nonzero counts preserve the previous behavior.
Non-blocking
A focused zero-color-attachment regression would strengthen coverage. The current PR changes only the implementation file, so the exact null-pointer case is not directly asserted by a new test.
Sources
- Implementation at the reviewed head
- Author's exact-head GPU and validation report
- Current contribution rules
Not verified
I did not independently run the GPU tests, validation layer, or a title. The test results above are author-reported. The unrelated agc_driver_pipeline_journal environment failure remains visible on both baseline and candidate.
Source correction — test report linkThe exact-head test report citation in my review should point to the author's 2026-10-29 06:26 UTC update, not the earlier comment ID. The verified link is the updated base/head GPU and validation report. The review outcome and stated findings are unchanged. |
|
Static review plus an independent Linux/AMD run on both trees per the repo AGENTS AGC protocol (CachyOS, kernel 7.2.10, GCC 16.2.1, Ninja 1.13.2, Release, What I checked
Paired AGENTS-protocol run (VVL env) Fixtures exercising the pipeline-library link path (
Comparing logs, no new validation errors. Both trees show 0 Note on run history (transparency) An earlier pass of mine reported Not covered The specific |
|
@blawesom thanks for the review and the paired run. You're right about the gap: no fixture links a depth-only merged-coverage pipeline with AI-assisted: yes (Claude Code) |
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (4, not counted against this PR)Validation IDs already present on main (4 IDs in 404 tests)Skipped (39)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
This PR's first two commits reached
mainthrough #8115 as identical patches:LinkPipelineFromLibrariescopies a linked pipeline's attachment sample counts and chains them to the link's and relink's create info. What is left is the follow-up from @MarcZX10's review:Pipeline.cppgives a depth-only merged-coverage pipeline aVkAttachmentSampleCountInfowithcolorAttachmentCount0 and a nullpColorAttachmentSamples, which Vulkan allows.memmove, which requires non-null pointers even for 0 bytes.attachmentSamplesalready chains a nullpColorAttachmentSamplesfor it.Tested
Current head
5d1e214ce3(the one commit, rebased ontomain52c09d567dwithout conflicts;git range-diffshows it unchanged), Ubuntu 26.04.1, GCC 15.2, RTX 4070 Ti, NVIDIA 615.71.09:agc_driver_pipeline_journalfails onmaintoo, as described below.main52c09d567dand at this head: 85 of 86 on both, with the same journal failure. Neither log has a missing-layer line, the layer checked 104 devices on each side, and neither has aVUID-message.Previous head
e6d81fbe9b(onmain3a2b919816), same machine:pipeline_library|^agc_driver_prepared|^agc_driver_graphics$|sample_coverage|msaa|multisample|pipeline_cache|journal, 85 pass in each of 3 runs.agc_driver_pipeline_journalfails the same way onmain3a2b919816. On this machine its 11th Vulkan device can't load the NVIDIA driver (libnvidia-tls.so.615.71.09: cannot allocate memory in static TLS block, withVK_LOADER_DEBUG=error) and falls back to llvmpipe. It passes withGLIBC_TUNABLES=glibc.rtld.optional_static_tls=8192, so it isn't this change.VK_LAYER_PATH=<LunarG SDK 1.4.363>/layers VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest -Vover the same 86, onmain3a2b919816and on this head.was not found but was requested by env var VK_INSTANCE_LAYERS.VUID-messages on either.Previous head
6346c5f2fe(onmaincf528e7): on Ubuntu 26.04.1, GCC 15.2, RTX 4070 Ti, 615.71.09, the 71 tests matchingpipeline_library|^agc_driver_prepared|^agc_driver_graphics$|sample_coverage|msaa|multisample|pipeline_cache|journalpass 3 of 3. Khronos validation 1.4.363 over the same 71 (VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 ctest -V) gives 71 of 71 onmainand on this head. Neither log has a layer-missing line. Both sides report the same pre-existing 14VUID-VkPipelineColorBlendStateCreateInfo-pAttachments-00605, and nothing is new in the head.Windows, on the current head
e6d81fbe9band onmain3a2b919816: Windows 11 VM, GeForce GT 1030 (NVIDIA R580), WinLibs GCC 15.2, Release,ANYPS5_REQUIRE_VULKAN=1. The 88 tests matching the same regex there pass 3 of 3 on both,agc_driver_pipeline_journalincluded. (On the earlier headff61956e21and its merge base eb8dcfe, the 68 tests the regex selected then passed 3 of 3 on both.)@ycbuzi ran the previous head on AMD (comment):
6346c5f2femerged ontomain1da5d1a, Windows 11 Home, WinLibs GCC 15.2.0, Release, AMD Radeon RDNA2 iGPU (driver 32.0.21030.2001), withANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1. The same regex selects 72 tests there, and they pass 72 of 72 onmainand 72 of 72 with this PR. The validation layer was not run on that device.Earlier results, on the three-commit heads named in them:
Ubuntu 24.04, GCC 13.3, NVIDIA RTX 4070 Ti (595.99.02).
Khronos validation, as CONTRIBUTING.md asks, with the LunarG SDK's 1.4.363 layer loaded through
VK_LAYER_PATH:VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ANYPS5_NO_SHADER_CACHE=1 ANYPS5_REQUIRE_VULKAN=1 ctest -V -R sample_coverage.main1126abb reportsVUID-vkCmdDraw-multisampledRenderToSingleSampled-07285×2. With this change it reports nothing, and neither log reports the layer missing. That holds both for 6bf4a39, which chained the counts underVkPipelineLibraryCreateInfoKHR(outside its allowedpNextchain, as @ebosido444-ai pointed out), and for c46005c, which chains them to the create info.With
APS5_NO_GPL=1,mainalready reports nothing, so only the library path was affected.The 58 tests matching
pipeline_library|^agc_driver_prepared|^agc_driver_graphics$|sample_coverage|msaa|multisample|pipeline_cache|journalpass with this change, as onmain.e9c9857, after @MarcZX10's review: the color sample counts are copied only when
colorAttachmentCountis non-zero. A depth-only pipeline'sVkAttachmentSampleCountInfoNV(zero colors, and a null array, asPipeline.cppbuilds it) then never readspColorAttachmentSamples. On Ubuntu 26.04.1 (GCC 15.2, NVIDIA 615.71.09), the same 58-test set passes 2 of 2 on this head. Khronos validation 1.4.363 oversample_coverage: the merge base 1126abb reportsVUID-vkCmdDraw-multisampledRenderToSingleSampled-07285×2. This head reports no VUID, and neither run reports the layer missing.Overlaps (checked 10-28 against the current heads): neither still edits the lines this PR changes.
6275369b6cmerges with this PR without a conflict.3339ddd3afpredates the sample-count copy this guard sits in (7c63ee6970, which reachedmainwith feat(agc): draw one-sample color targets beside a multisampled depth target with sample 0's coverage #8115). It doesn't edit these three lines. It conflicts withmainitself inPipelineLibrary.cppand other files, so its rebase meets this code either way.So there's no order to agree, and
Depends onstays none.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)🤖 Generated with Claude Code