Repository navigation
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.
|
BunnyFl
left a comment
BunnyFl
left a comment
There was a problem hiding this comment.
DefineInputs currently adds each extra builtin only under the condition that needs it (laneCount == 2 || spannedWave, ldsLock, LdsInDeviceMemory). The PR declares WorkgroupId, LocalInvocationIndex and GlobalInvocationId in every compute module instead.
- Which lowering adds the
GetBuiltinafterCollectBuiltinInputs(ShaderInfoCollector.cpp:650) ran? If it's one pass, re-collecting there, or adding the input under that pass's condition as the other cases do, would keepInfo().inputsaccurate. Unconditional declarations hide the next stale-info case. - The change alters every compute module's SPIR-V, so
RecompileResult::Identity, every compute pipeline journal entry and the disk cache entries change. Fine if intended; worth a line in the PR. - There's no regression test for the failing shape. A recompile/emission test of a compute program that reaches the failing lowering (the PPSA08330 shape, without the title ID in the test name) would fail on main with
stage input 12 was not declared.
Not run here.
✅ NVIDIA GPU test: passedPR head
Unstable on main (2, 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)Validation IDs already present on main (5 IDs in 484 tests)Skipped (45)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. |
DefineInputs now declares the compute invocation builtins the IR reads instead of every compute module declaring all three. Modules that do not read them keep their previous SPIR-V and cache identity.
f764187 to
594674b
Compare
|
Follow-up on Added Focused tests pass; |
✅ NVIDIA GPU test: passedPR head
Failing on main too (3, not counted against this PR)Validation IDs already present on main (13 IDs in 548 tests)Skipped (49)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. |
BunnyFl
left a comment
BunnyFl
left a comment
There was a problem hiding this comment.
Re-reviewed at 594674b, which answers all three points from my earlier comment:
- Declarations now follow the
GetBuiltininstructions in the final IR at emission. A builtin a lowering adds afterCollectBuiltinInputsgets declared, and a compute module that reads none of them keeps its SPIR-V and cache identity. - The regression test is there.
Windows 11, WinLibs GCC 15.2.0 r7, Release, AMD Radeon RX 9070 XT (driver 32.0.32015.7029), Vulkan SDK 1.4.363 validation layer, ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation. Merged into upstream/main 93dbd0f:
agc_compute_builtinspasses. Negative control: with main'sSpirvEmitterHelpers.cppit fails withSPIR-V module emission failed: stage input 12 was not declared.agc_driver_image_packed_horizontal_gatherandagc_driver_graphicspass on this branch and on main, none skipped. Neither log has the missing-layer message, aVUID-or aSYNC-HAZARDID.
That adds an AMD run to your NVIDIA one.
|
AMD RDNA 2 on RADV, which the Tested section and the NVIDIA report don't cover. No regression, and the newly declared builtins compile and dispatch on RADV. One limit: none of the GPU tests below reach the changed path, so the RADV evidence for it comes from the separate compile check. Setup: RX 6700 XT ( Commits: head
New test:
Coverage of the change: I added a temporary print where The new modules on RADV: to reach that path, I temporarily dumped the modules
Same on both sides, unrelated to this PR:
Not tested: God of War Ragnarök or any other title, Windows. AI-assisted: yes |
|
Checked the current head against the review:
AI-assisted: yes |
|
No code change was needed. This head is already approved at Verification on
The earlier review items are present in this head: |
|
Current head
The cancelled |
|
Re-checked the current approved head Checks on
No new change was pushed. |
✅ AMD GPU test: passedPR head
Unstable on main (2, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (8, not counted against this PR)Validation IDs already present on main (4 IDs in 558 tests)Skipped (67)ReproduceAutomated build and test on a contributor's AMD machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
|
Re-checked the approved head On this head:
No source change was needed and nothing was pushed. AI-assisted: yes. |
|
Verified the current head |
|
No changes needed: the current head |
|
Re-verified at head Locally: |
|
Re-verified approved head The review points are already in this head:
No source change was needed and the approved head was not rebased, so nothing was pushed. AI-assisted: yes. |
|
The Re-ran locally on this head:
I did not re-run the GPU validation gate here; the head is unchanged. AI-assisted: yes. |
|
The watchdog flagged the earlier Re-checked locally on |
What
Compute shaders that read
GlobalInvocationIdorWorkgroupIdcould fail module emission withstage input 12 was not declared(WorkgroupId) orstage input 15 was not declared(GlobalInvocationId), even though the IR uses them.DefineInputsonly addedWorkgroupIdwhenlaneCount == 2or the wave was spanned, andGlobalInvocationIdwas left toCollectBuiltinInputs; when a lowering pass introduces either builtin after collection, the interface variables are missing.DefineInputsnow adds only the compute invocation builtins that the IR actually reads, plus the existing lanes and LDS conditions. Unused compute builtin inputs are legal, and the lowering paths that combine local and workgroup ids find their inputs. A compute module that reads none of them keeps its previous SPIR-V and shader cache identity.God of War Ragnarök (PPSA08330) fails on
0x1058daf400and several0x10527...compute programs withstage input 12; with this change those programs prepare, and the run advances to a different, unrelated blocker (a vertex shader without a position export).Tested
Linux x86-64 (CachyOS), GCC 16.2.1, Release, NVIDIA GeForce RTX 4080 SUPER (driver 615.78.8.0):
agc_compute_builtins,agc_spirv_module,agc_guarded_memory_loads,agc_tessellation_lowering,agc_validate_programandagc_shader_disk_cachepass.agc_driver_image_packed_horizontal_gatherandagc_driver_graphicspass withANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationonmainand on the change; neither log hasVUID-lines.stage input 12/15failures disappear from the log and the game reaches the next shader problem.tools/check_conventions.py --base origin/mainandgit diff --checkare clean.tools/pr_overlap.pyreports only a shared file with #12619 and #12764 (no shared function; #12764 also uses these builtins).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)