Repository navigation
fix(agc): build a pipeline with a fragment shader interlock whole, not from libraries - #9185
Conversation
|
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.
|
|
RTX 4090 (Ada) on Windows, head
So on NVIDIA's driver the ordering held in pipelines linked from libraries: with every draw fast-linked, NVIDIA GeForce RTX 4090, driver 617.42, Windows 11 Pro 26200, WinLibs GCC 15.2.0 r7, Release, AI-assisted: yes (Claude Code) |
|
Thanks for the NVIDIA runs, especially the APS5_NO_PIPELINE_LTO=1 row; that confirms the ordering bug is RADV's fast link, not something every driver does. I'd still like to keep whole pipelines for interlock shaders on every driver. On VK_DRIVER_ID_NVIDIA_PROPRIETARY, PipelineLinkTimeOptimizationUsable is false, so the fast link is never replaced by an optimized relink, and these pipelines would keep the fast-linked vertex stage's extra VGPRs for the whole run. Building them whole costs one compile at the first draw, and these shaders are rare. It also keeps the drivers nobody has measured yet (AMDVLK, AMD on Windows, ANV) safe without a driver check that needs a version limit once mesa#16653 ships. I've added your result to the debt entry so the unmeasured list is accurate. If the first-draw compile ever shows up in a trace, limiting it to RADV is an easy follow-up. |
jotitadev
left a comment
jotitadev
left a comment
There was a problem hiding this comment.
A Metal / MoltenVK data point for the "drivers nobody has measured yet" list in the debt entry.
Machine: Apple M2 Max, macOS 27.2, x86_64 under Rosetta 2. MoltenVK 1.4.2 (Vulkan SDK 1.4.363.0). Release, BUILD_TESTING=ON.
Base/method: PR #8658 feat/macos-port @ 4ebe71f2e5 (current main 0c6a5bd593 + the macOS port), not main. This PR applied on top, clean.
On MoltenVK the device reports pipelineLibrary=0, so graphicsPipelineLibrary is off and libraryKeys() already returns false for every pipeline. The new FragmentInterlock() branch therefore changes nothing here (no pipeline is ever linked from libraries), and the new Require(!device->GraphicsPipelineLibraries() || ...) is satisfied by the left operand. So the change is a no-op on this host and the RADV fast-link ordering bug this PR fixes cannot occur on MoltenVK — Metal is not a vendor that can exercise the regression.
It is also worth noting that agc_driver_pixel_interlock itself does not pass on MoltenVK, with or without this PR, for an unrelated reason: SPIRV-Cross emits MSL that Metal rejects at pipeline creation —
error: type 'const constant spvDescriptorSetBuffer0 &' is not valid for attribute 'buffer'
note: field with illegal attribute 'raster_order_group' declared here
coherent device _17* buffers_coherent [[id(2), raster_order_group(0)]][1];
so the interlock test is blocked on Metal before this PR's path is reached. No regression from this PR on Apple.
GPUs tested: Apple M2 Max (MoltenVK 1.4.2).
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Thanks for checking on Apple. You're right that it's a no-op there: with The Metal compile failure ( |
There was a problem hiding this comment.
The reproduction and evidence are unusually useful here: the report isolates the failure to the fast-linked library path, the regression check fails without the production change, and the described main/change validation-layer runs show matching VUID counts. I could not independently run the GPU test in this environment because it has no Vulkan device or tools; the hosted product jobs are skipped, so I’m treating the GPU results as author-reported. Before this is merge-ready, please address the repository process items: - The branch is 599 commits behind current upstream main (2 commits ahead). CONTRIBUTING requires keeping PR branches current. Please update to current main, rerun the focused validation-layer A/B suite on that base, and refresh the overlap report. - The overlap report identifies open PR #7888 changing the same libraryKeys condition and #9489 touching the same Pipeline.cpp include area. Your description explains how to preserve both #7888 conditions, but the PR discussion does not record an agreed order; CONTRIBUTING asks authors to coordinate overlapping work and declare the dependency/order in the checklist. Please coordinate with both authors and update Depends on to match whichever must land first. My local current-main convention check and git diff --check pass, and the merge-tree is clean. Hosted conventions, eligibility, labels and report pass; Linux, Windows and product-test jobs are skipped. I’m requesting changes for the stale base and overlap coordination only; I did not find a separate code correctness defect in this diff.
I’ve noticed that holding this PR because of #5239 isn’t necessary. It’s fine for PRs to keep coming in; they’ll be handled in due course. Please disregard my earlier request to hold or defer it.
2237de6 to
9eeac40
Compare
|
@MarcZX10 rebased onto On mesa#16653: the first fix in the issue thread doesn't cover the fast-linked path yet (92 of 120 fast-linked runs still failed with it), so the workaround is still needed. |
|
I’ve noticed that my earlier request to hold this PR based on #5239 was unnecessary. It’s fine to keep submitting PRs; maintainers will handle them in due course. Please disregard that request. |
|
Thanks for refreshing the A/B runs and overlap report. I checked head |
9eeac40 to
00042e1
Compare
|
@MarcZX10 rebased onto Gooshy hasn't answered on #9489 yet, so the description now has a concrete plan. #9489 currently conflicts with |
00042e1 to
5ce1b55
Compare
Dismissing a stale review: the requested changes were addressed after it (new head and/or updated description with the asked-for runs) and the merge queue blocks on it. Please request changes again if anything is still open.
What
agc_driver_pixel_interlockfails now and then when the test suite runs in parallel. With 12 copies at once on an RX 7900 GRE (RADV, Mesa 26.2.5), 14 of 240 runs failed. Every failure was the first draw of the process:wave64 pass 0: pixel N counted 63 of 64 overlapping read-modify-writes, withNalways on an 8x8 tile corner. So one whole wave64 tile lost one increment.The first draw is the one that uses the fast-linked graphics-pipeline-library pipeline. After it, the background
LINK_TIME_OPTIMIZATIONrelink takes over. The pipeline modes give:APS5_NO_PIPELINE_LTO=1(every draw fast-linked)APS5_NO_GPL=1(whole pipelines)RADV_DEBUG=shaders,epilogsshows the difference. In the whole pipeline, ACO putss_waitcnt_vscnt null, 0x0between the counterbuffer_store_b32and the finalexp mrt0 ... done. On GFX11 thatdoneexport ends the primitive-ordered section, so the store lands before the next overlapping wave is let in. In the fragment-shader library, ACO moves the export into a separate PS epilog. The main part stores and then jumps to the epilog (s_setpc_b64) without the wait, and the epilog is onlyexp mrt0 ... doneands_endpgm. The next wave covering the same pixels then reads the count before the store lands. This is a driver bug: a pipeline linked from libraries must behave like the whole pipeline. It is reported upstream as mesa#16653. The workaround below can be removed once the fix has shipped. The fix is in review as mesa!45546: with its revisiond71ecb1, ACO waits for the store before the jump to the epilog, and the loaded runs ofmain's test (without this change) fail 0 of 120 fast-linked and 0 of 240 by default.This change builds a pipeline whole when its fragment shader declares a fragment shader interlock capability.
FragmentInterlockinPipeline.cppreads the module's leadingOpCapabilitywords, andlibraryKeysdeclines such pipelines, as it already does for rect lists, per-face depth bias and barycentric patches. These pipelines lose the fast link on every driver and are compiled whole at the draw. Primitive-ordered pixel shaders are rare, so this costs one whole compile per such pipeline. A new debt entry, "Fragment shader interlock pipelines", records the reason, the cost and which drivers' fast links are measured.The test now also requires that none of its pipelines was linked from libraries when the device uses them. Without the
Pipeline.cppchange, that check fails on every run, loaded or not, so the regression no longer depends on load.Overlap: on
main24c6cc33f0,pr_overlappaired this change with #7888, and onb7d1d47a5falso with #9489. #7888 has since merged; the rebase ontoe13027a1f3keeps itsfunctionLdsBuffercondition and this change's condition on the samelibraryKeysline. The remaining overlaps are only textual:#include <string>where this change adds#include <spirv/unified1/spirv.hpp>; the merged list keeps both. ItslibraryKeyschange is to the key contents, not to which pipelines use libraries.pr_overlaprun) adds(state.stages.tessellation && !context.tessellationPipelineCache)to the samelibraryKeysline. It is independent of this change, and the merged line keeps both conditions. Its otherPipeline.cpphunk, the pipeline cache choice in the constructor, merges cleanly. The order is agreed in its thread: noDepends oneither way, and the one that merges second rebases.Neither depends on this change or this change on them, so the one that merges second rebases, as @mazen320 agreed for #7888 (comment). @jotitadev agreed to the same for #10046 (comment); it is proposed in #9489, without an answer yet. If this change merges first, #9489's rebase keeps both includes. If #9489 merges first, I rebase this change onto it and rerun the loaded
agc_driver_pixel_interlockruns and the validation-layer comparison before it merges. Of the pull requestspr_overlapcould not compare:maininPixelInterlock.cpp, while this change adds one line after the draw loop.Pipeline.cpp, only feat(agc): look up table images at run time through a device image table on the GPU heap (APS5_GPU_PATH) #7642 touches library handling, and it changes the pipeline layout flags, not whether a pipeline is built from libraries. feat(agc): draw tessellation followed by an NGG geometry stage #6612, feat(agc): draw point lists on the vertex path with their point size #6639 and feat(agc): rasterize geometry points with their point size #6801, new in the list, change point and tessellation drawing there. perf(agc): build the pipelines of a submission ahead on a compile pool #5925 and fix(shader): read a 2D MSAA T# through DIM 2D as its sample 0 #7803 touch graphics pipeline libraries only in tests (a mock device intests/Graphics.cppand a test pass intests/Recorder.cpp).Tested
Arch Linux, AMD Radeon RX 7900 GRE (RADV NAVI31, Mesa 26.2.5), Release, on
maincaa089fd73.agc_driver_pixel_interlock, 12 copies at once, withMESA_SHADER_CACHE_DISABLE=trueso every process compiles its pipelines:mainAPS5_NO_PIPELINE_LTO=1(every draw fast-linked)With the driver's shader cache disabled the background relink lands later, so more draws use the fast-linked pipeline than the 14 of 240 failures first measured with the cache on.
With only the test change (no
Pipeline.cppchange), the test failed 3 of 3 runs alone, each at the new check.ctest --test-dir build -R 'agc|shader|recompiler' -V -j6withANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation, onmainand on this change: both 884 of 885, with the same failure, theagc_driver_tessellationsegfault (agc_driver_tessellation segfaults on RADV with Mesa 26.2+: nir_opt_varyings corrupts the TCS input offset's use list (Mesa bug) #5444). Both logs have the same four VUIDs with the same counts, so the change adds no validation errors, and neither log reports the validation layer missing.NVIDIA GeForce RTX 4090 (driver 617.42, Windows 11), tested by @MaxLikesCode in this comment:
agc_driver_pixel_interlockpassed onmainand on this change, with and without the validation layer and with noVUID-; 12 copies at once failed 0 of 240 runs on both, by default and withAPS5_NO_PIPELINE_LTO=1. NVIDIA's fast link keeps the ordering, and the debt entry now says so. The workaround still applies on every driver: NVIDIA never relinks with link-time optimization, so a whole pipeline also avoids the fast link's extra vertex-stage VGPRs there, and AMDVLK, AMD on Windows and ANV are not measured.python3 tools/check_conventions.py --base upstream/mainpasses.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)