Repository navigation
Conversation
…one-lane waves Vertex and local programs on a device without vertex subgroups run as one-lane waves, where a shuffle returns the invocation's own value. v_readlane_b32 checked its lane only when a constant named it: a lane held in an SGPR read the invocation's value whatever lane it named, where the console reads that lane's VGPR (RDNA 2 ISA 12.12, V_READLANE_B32: the lane is S1 % 64 in wave64). On a one-lane wave a register-selected lane must now be the s_ff1 of a mask built from EXEC and ballots, as in a waterfall loop. Such a mask holds no lane but the invocation's own, so the read is its own value; the -1 of an empty mask (RDNA 2 ISA 12.3, S_FF1_I32_B64) reads it too. Any other selector throws through FailOutsideHostSubgroup. Accepted programs emit the same SPIR-V as before. The one-lane rules leave the vertex entry debt line for an entry of their own, which also lists the cross-lane reads still unchecked there: DPP moves, ds_swizzle_b32, ds_bpermute_b32 and v_permlane16_b32.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — looks good
The exact head’s lane guards and regression cases are internally consistent, and the author reports the focused shader tests passing on this head.
What I ran
Reviewed head 2928046282310a415ae3c922ff7b035dbf852706 against base 0f302813cebfc09e3e0b6f8fced15262d186879e; the dependency #14068 is included, and each PR’s merge preview against that base was clean. Environment: Debian Linux 6.1, Python 3.11.2, GCC 12.2.0; no C++ compiler or build was run.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14072 --output-dir /tmp/anyps5-14072-review
git diff --check 0f302813cebfc09e3e0b6f8fced15262d186879e...2928046282310a415ae3c922ff7b035dbf852706
python3 tools/check_conventions.py --base 0f302813cebfc09e3e0b6f8fced15262d186879e --head 2928046282310a415ae3c922ff7b035dbf852706 --title "fix(shader): throw for DPP, ds_swizzle, ds_bpermute and v_permlane16 reads of another lane on one-lane waves" --body /tmp/pr-14072-body.md
python3 tools/pr_test_quality.py --root /tmp/anyps5-14072-review/head-292804628231-ffa8523b --json core/libs/prx/libSceAgcDriver/tests/ShaderMemory.cpp
git show 2928046282310a415ae3c922ff7b035dbf852706:core/libs/prx/libSceAgcDriver/CMakeLists.txt | grep -n 'tests/ShaderMemory.cpp\|ShaderMemory.cpp'
gh api repos/boykopovar/AnyPS5/commits/2928046282310a415ae3c922ff7b035dbf852706/check-runs --jq '[.check_runs[] | {name,status,conclusion}]'Results
| Check | Result |
|---|---|
| Patch whitespace | Passed: git diff --check emitted no output. |
| Repository conventions and PR template | Passed. |
| Dependency and merge previews | Passed: #14068’s exact head is included; both PRs preview cleanly against the current base. |
| Test-quality heuristic | One possible-registration warning; verified against the PR head’s CMake file, which registers tests/ShaderMemory.cpp in agc_shader_memory_tests. The warning is a false positive. |
| Source review | No blocking issue found in the selector/address walk or the DPP, DS-swizzle, bpermute, and permlane16 lane-0 guards. The added cases cover accepted own-lane forms, rejected cross-lane forms, and 32-lane subgroup compilation. |
| Author-reported focused tests | The PR description reports its targeted CTest set passing three times on this head and a prior main ancestor, plus validation-layer runs without VUID output. These are author-reported, not independently rerun here. |
| Hosted checks at final refresh | Windows and ASAN passed; eligibility, conventions, library portability, intake, labels, and report passed. Linux was skipped. Two relinker jobs were in progress and two macOS relinker jobs queued; these are not shader-focused test results. |
Raw outputs and relevant report excerpts
git diff --check output (empty):
check_conventions.py:
All conventions checks passed.
Dependency report:
Dependency order: #14068 → #14072
- PR #14068: **clean** against current base; head `5e086eaac896fb794a8a59c947e3427ca6ebde43`
- PR #14072: **clean** against current base; head `2928046282310a415ae3c922ff7b035dbf852706`
- #14068 → #14072: **head_included**
Test-quality heuristic output:
{
"findings": [
{
"path": "core/libs/prx/libSceAgcDriver/tests/ShaderMemory.cpp",
"line": 1,
"kind": "possible-unregistered-cpp-test",
"message": "Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage."
}
],
"negative_control_candidates": [],
"limits": [
"Heuristics can miss weak assertions and can flag valid tests; inspect every finding.",
"A test that passes against base is not by itself proof that the PR test is inadequate.",
"Negative controls are only candidates until the test actually runs against base and fails by assertion rather than import/setup error."
]
}CMake registration at the reviewed head:
787 add_test_executable(agc_shader_memory_tests EXCLUDE_FROM_ALL
788 tests/ShaderMemory.cpp
789 Execution/src/ShaderMemory.cpp
Hosted-check snapshot (the response to the command above):
[
{"name":"write-watch","status":"completed","conclusion":"skipped"},
{"name":"register-context","status":"completed","conclusion":"skipped"},
{"name":"linux","status":"completed","conclusion":"skipped"},
{"name":"relinker-asan","status":"completed","conclusion":"success"},
{"name":"relinker (macos-latest, clang++, x86_64)","status":"queued","conclusion":null},
{"name":"relinker (ubuntu-latest, g++)","status":"in_progress","conclusion":null},
{"name":"relinker (windows-latest, g++)","status":"in_progress","conclusion":null},
{"name":"relinker (macos-latest, clang++)","status":"queued","conclusion":null},
{"name":"windows","status":"completed","conclusion":"success"},
{"name":"intake","status":"completed","conclusion":"success"},
{"name":"labels","status":"completed","conclusion":"success"},
{"name":"conventions","status":"completed","conclusion":"success"},
{"name":"report","status":"completed","conclusion":"success"},
{"name":"eligibility","status":"completed","conclusion":"success"},
{"name":"library-portability","status":"completed","conclusion":"success"}
]The precheck also attempted a changed-export ABI audit, but that helper was absent from its isolated PR checkout; this PR changes no exports, so that audit is not applicable. I did not treat that helper-launch error as a PR failure.
Findings
Blocking
None found in the reviewed source.
Non-blocking
The focused shader tests and validation-layer runs are reported by the author; I did not independently reproduce them. The author reports testing against e8c25dd3ba, an ancestor of the current base; the only change between that test base and current main touching these tests is CMake registration metadata.
Sources
- Lane selector and address proof at the reviewed head
- Cross-lane guards at the reviewed head
- Added regression cases and test registration at the reviewed head, CMake target
- Author’s reported test details
Not verified
I did not build or compile the recompiler, run CTest, or exercise a GPU/console locally. The final hosted snapshot still had relinker jobs in progress or queued; Linux was skipped. The PR description’s hardware and validation-layer results were not independently confirmed.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
I am correcting my earlier approval after finding that the inherited selector proof can accept a register-selected lane other than the current lane.
What I ran
Reviewed head 2928046282310a415ae3c922ff7b035dbf852706 against base 0f302813cebfc09e3e0b6f8fced15262d186879e; the reported merge preview was clean. Environment: Debian Linux 6.1, Python 3.11.2, GCC 12.2.0; no build or CTest was run.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14072 --output-dir /tmp/anyps5-14072-review
python3 tools/pr_stack_check.py 14072 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-14072-review/stackResults
| Check | Result |
|---|---|
| Earlier no-build precheck and stack preview | The exact head/base above were checked; the dependency/merge preview was clean. |
| Selector proof | Blocking source issue confirmed: an empty FindILsb32 input can produce -1/lane 63, and the SelectU32 shortcut does not validate that its condition is related to the low mask. |
| Hosted CI | Windows relinker job is failing on this head; Linux and macOS relinker jobs passed; remaining queued/skipped jobs are not evidence of a full platform matrix. |
| Earlier review | My previous approval is superseded by this request for changes. |
Raw outputs
The precheck report and raw command output were generated under /tmp/anyps5-14072-review. No build or CTest output exists because neither was run.
Negative control
| Assertion | Mutation or input | Did the new test fail? |
|---|---|---|
| A one-lane host must not substitute lane 0 for a possible guest lane 63. | FindILsb32(0) as the lane selector, and SelectU32(unrelated_false, FindILsb32(1), 0xffffffff). |
These false-acceptance cases are not covered by the tests I inspected and were not run locally. |
Findings
Blocking
The inherited OwnLaneSelectorWalk::selector() proof accepts FindILsb32(word) when word is proven to contain no bits outside bit 0, but that proof accepts word == 0. The ISA S_FF1_I32_B32 result for zero is 0xffffffff (-1); EmitReadLane masks it with waveSize - 1, yielding lane 63 for a wave64 guest. In a one-lane host subgroup, EmitSubgroupShuffle copies the current value, so an executing guest lane 0 can receive lane 0's value where the console reads lane 63.
The SelectU32 shortcut has a separate gap: it checks the true/false values but never verifies that the select condition tests the same low mask. An unrelated false condition can therefore select 0xffffffff even when bit 0 is present. Please make the proof fail closed unless it establishes lane 0 for every executing invocation, including non-emptiness and the relationship between the condition and both branches. Add regressions for an empty low mask while lane 0 remains active and for the unrelated-condition case.
Non-blocking
The Windows relinker CI job is failing. Its failure is not the basis of this request, but should be diagnosed and reported with the next update.
Sources
- Selector proof at this PR head
- Lane masking at this PR head
- Single-lane host shuffle behavior at the base
- AMD RDNA 2 ISA reference for
S_FF1_I32_B32
Not verified
No build, CTest, GPU, or console run was performed. Hosted CI does not substitute for the missing regression cases. The Windows failure's cause has not been diagnosed here.
To get this to a good state
Fix the selector proof and add both regressions, then diagnose the Windows job. I will rerun the same relevant no-build checks on the new head.
✅ 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 (3 IDs in 387 tests)Skipped (48)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. |
|
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.
|
|
@frankea the lane-0 sources look right at
|
…ane_b32 lane on one-lane waves The selector proof accepted the s_ff1 of a mask built from EXEC and ballots whenever that mask held no lane but the invocation's, empty masks included. s_ff1 of an empty mask is -1 (RDNA 2 ISA 12.3, S_FF1_I32_B64), which v_readlane_b32 reads as lane 63, the read a constant lane 63 already refuses. It also took Select(c, FindILsb32(mask), -1) without checking that c tests that mask. The mask must now hold the invocation's bit wherever the read runs: it is the entry EXEC, or a branch on the way to the read tested it for zero, which covers the back edge of an s_ff1 waterfall and a guard before one. The select must test mask != 0. Selects and phis of selectors are no longer accepted. New cases: a waterfall over an unguarded compare result and an s_cselect_b32 of s_ff1_i32_b32 of EXEC or -1 throw; the same waterfall behind s_cbranch_scc0 and a plain s_ff1_i32_b32 of EXEC build. Both rejected cases build with the previous selector proof.
…e ds_bpermute_b32 address The review of boykopovar#14072 found the sum and OR rules of the address proof unpinned: weakening either let a ds_bpermute_b32 whose address added the vertex index to a lane-0 term build and read the invocation's own value. Guest code reaches neither rule. Its lane id comes from v_mbcnt as a BitCount, which the proof never accepts, so the only register address it needs is the one the translator builds for image_sample_*_cd. The proof now accepts just that: the lane id, an AND with it, or the quad's first active lane, shifted left by 2. Sums, ORs, selects and offsets throw. Four new cases pin this: the v_mbcnt lane id plus the vertex index, the vertex index plus 32, the vertex index OR 32, and the vertex index at offset 128. Merged hull programs never reach these checks, because TessellationLowering's requireOwnLanes already throws for every cross-lane opcode before emission. The lane id is therefore accepted in every stage, and the v_readlane_b32 call site is back to boykopovar#14068's.
|
@oneandonlydean thanks, both are addressed in 6617bb0.
Tested on 6617bb0: the 21 MarcZX10's requested change is in the selector proof this branch takes from #14068. It is being fixed there, and I will rebase this branch onto it. |
…reads of another lane on one-lane waves
Vertex and local programs on a device without vertex subgroups run as
one-lane waves: the invocation is lane 0 and a shuffle returns its own
value. DPP moves, ds_swizzle_b32, ds_bpermute_b32 and v_permlane16_b32
had no lane check there, so a value the console reads from another
lane's VGPR came back as an inactive lane's (0, bound_ctrl or the old
value) or, with fetch-inactive, as the invocation's own.
On a one-lane wave they now throw through FailOutsideHostSubgroup when
lane 0 reads another lane:
- DPP: lane 0's source from DPP_CTRL (RDNA 2 ISA 13.3.9, Table 91:
quad_perm, row_shl, row_ror, row_mirror, row_half_mirror; row_share
0x150-0x15F and row_xmask 0x160-0x16F as LLVM's dpp16_ctrl and
SIDefines.h DppCtrl define them) or DPP8 LANE_SEL0 (13.3.10, Table
92). row_shr gives lane 0 no source, so its scan steps still build.
- ds_swizzle_b32: lane 0's source from the offset (12.13.1: or_mask ^
xor_mask in the 32-thread mode, offset[1:0] in the 4-thread mode;
rotate mode as the emitter decodes it).
- ds_bpermute_b32: a constant address must name lane 0
((ADDR + OFFSET) / 4, 12.13). A register address must be the one the
translator builds for image_sample_*_cd, which takes its derivatives
from the quad's first active lane, the invocation's own on one lane:
OwnLaneSelectorWalk proves it is the lane id, an AND with it, or
Select(mask != 0, quad + FindILsb32(mask), lane) over a ballot mask,
shifted left by 2. Guest code reaches the lane id only through
v_mbcnt, a BitCount the walk never accepts, so every register address
a guest program writes throws.
- v_permlane16_b32: lane 0 reads S1[3:0] (12.12, lanesel = {S2, S1}),
so S1 must be a constant with those bits 0. v_permlanex16_b32 was
already checked.
Merged hull programs never reach these checks, because
TessellationLowering's requireOwnLanes refuses every cross-lane opcode
before emission. Accepted programs emit the same SPIR-V as before.
6617bb0 to
4d7358d
Compare
|
@MarcZX10 the selector proof you flagged comes from #14068 and is fixed there in |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The revised lane-address proof and added refusals address the prior findings, and I found no remaining blocking issue at this head.
What I ran
Reviewed head 4d7358d6acffae2b952295dc0db17e9c5f6da593 against PR base ca073ec3ad0d766ea9f4658e418a5c977001a353; the no-build precheck recorded a clean merge preview against current main ca073ec3ad0d766ea9f4658e418a5c977001a353. The head includes dependency #14068 at 283e3e773aecc5d9d744b223ff2d11331b11dd7e. I inspected the address proof and accepted/rejected one-lane test cases. No local C++ build or test was run.
python3 /root/.codex/skills/anyps5-pr-review/scripts/pr_snapshot.py 14072 --repo boykopovar/AnyPS5
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14072 --output-dir /tmp/anyps5-14072-review-new
gh api repos/boykopovar/AnyPS5/commits/4d7358d6acffae2b952295dc0db17e9c5f6da593/check-runsResults
| Check | Result |
|---|---|
| Diff and conventions | Passed |
| Current-main merge preview | Clean |
| Hosted Windows product job | Passed |
| Hosted Ubuntu relinker and ASan | Passed |
| Author-reported focused tests | 36 focused CTest entries passed 3 times on Windows and on main; 21 shader-memory entries passed 3 times on macOS. Not independently reproduced. |
| Hosted Linux product job | Skipped |
| Hosted Windows relinker | In progress at review time |
| Hosted macOS relinker jobs | Queued at review time |
Findings
Blocking
None found on this re-check.
Non-blocking
The focused Windows/macOS results are author-reported, not independently reproduced. The Windows relinker job is still running and the macOS relinker jobs are queued; the reported earlier Windows relinker timeout was on an older head and in unrelated compile-mode tests.
Sources
- Address proof and one-lane emission checks at exact head
- Accepted and rejected one-lane programs at exact head
- Required lane-selector fix in dependency #14068
Not verified
I did not run a local build, Linux runtime/GPU test, or console behavior. The hosted Linux product job was skipped; Windows relinker and macOS relinker checks were unfinished at the time of this review.
✅ NVIDIA GPU test: passedPR head
Failing on main too (4, not counted against this PR)Passed on retry (1, failed in the parallel run, passed when re-run with a 360 s timeout; not counted against this PR)Validation IDs already present on main (3 IDs in 387 tests)Skipped (48)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. |
✅ 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 (4, not counted against this PR)Validation IDs already present on main (4 IDs in 387 tests)Skipped (48)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. |
…-register-selector # Conflicts: # docs/dev/debt/shader.md
Tessellation evaluation programs without subgroups now take the one-lane path from boykopovar#13807, so verifySingleLaneDomainWave checks that a v_readlane_b32 at the lane v_readfirstlane_b32 takes from v5 throws there and builds on a 32-lane tessellation evaluation subgroup.
…ector' into fix/one-lane-cross-lane-reads
|
Linux / AMD RDNA 4 (RADV) paired validation run for current head Host & GPU: CachyOS Linux ( Environment:
AI-assisted hardware test run (Antigravity). |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — new head, looks good
The current head 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 includes #14068's current tessellation-evaluation regression test and debt update; I found no blocking issue in that addition or in the previously reviewed cross-lane changes.
What I ran
Reviewed exact PR head 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68. The PR's recorded base is 029463ceb62c97e3a82cfea423f884f158a4f023; current main is dc9dc7eaaeaf99b1c491d426e68bb999def3da16. The merge preview against current main was clean (264fdc10582a6a33022a48dbba961f20a6a7134f). Environment: Linux x86_64, Python 3.11.2, Git 2.39.5; no compiler invoked.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14072 --output-dir /tmp/anyps5-pr-precheck-14072-current
git diff --check dc9dc7eaaeaf99b1c491d426e68bb999def3da16...4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68
git merge-tree --write-tree dc9dc7eaaeaf99b1c491d426e68bb999def3da16 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68
python3 tools/check_conventions.py --base dc9dc7eaaeaf99b1c491d426e68bb999def3da16 --head 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 --title 'fix(shader): throw for DPP, ds_swizzle, ds_bpermute and v_permlane16 reads of another lane on one-lane waves' --body /tmp/anyps5-pr-precheck-14072-current/pr-body.md
python3 /marc/AnyPS5/tools/pr_test_quality.py --root /tmp/review-14072-4b3d --json core/libs/prx/libSceAgcDriver/tests/ShaderMemory.cpp
gh api repos/boykopovar/AnyPS5/commits/4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68/check-runs --jq '.check_runs[] | [.name,.head_sha,.status,.conclusion] | @tsv' | sort -uThe PR description reports 36 focused CTest entries passing three times on Windows for the branch and its pinned main baseline, plus 21 shader-memory entries passing three times on macOS. A separate RADV report on this exact head records 35/38 focused checks passing three times on both its main baseline and the PR head, the same two expected skips, the same Mesa tessellation crash on both trees, and zero VUID/SYNC hazards. I did not build or run these tests locally.
Results
| Check | Result |
|---|---|
| Current-head diff and integration | Passed; the current PR diff is clean, and Git's merge preview against current main is clean. The new verifySingleLaneDomainWave test is called by the test runner. |
| Repository conventions | Passed against current main. |
| Test registration | Confirmed in agc_shader_memory_tests.cmake. The workspace-local test-quality heuristic flagged the changed C++ path as possibly unregistered because it did not match the relative path in the CMake fragment; direct inspection confirms registration. |
| Hosted checks on exact head | Windows product job, conventions, labels, library portability, and relinker jobs passed. The Linux product job was skipped. |
| Focused Windows/macOS tests | The PR author reports the listed CTest runs passing; I did not reproduce them locally. |
| Linux/RADV paired run | PR comment #6552362448 reports 35/38 passes on both main and this exact head over three runs; the same two stage-subgroup skips and same Mesa #5444 tessellation crash occur on both trees, with zero VUID/SYNC hazards. |
| Local build and runtime tests | Not run. |
| No-build precheck helpers | The precheck could not launch its ABI and test-quality helper scripts because they were absent from the detached PR checkout. ABI auditing is not applicable to this change; I ran the available workspace-local test-quality heuristic separately and checked the registration directly. |
Unedited output from checks
git diff --check stdout (empty):
git merge-tree --write-tree:
264fdc10582a6a33022a48dbba961f20a6a7134f
Conventions checker:
All conventions checks passed.
Test-quality heuristic output:
{
"findings": [
{
"path": "core/libs/prx/libSceAgcDriver/tests/ShaderMemory.cpp",
"line": 1,
"kind": "possible-unregistered-cpp-test",
"message": "Changed C++ test source path was not found literally in CMake files; inspect registration, generated source lists, and workflow coverage."
}
],
"negative_control_candidates": [],
"limits": [
"Heuristics can miss weak assertions and can flag valid tests; inspect every finding.",
"A test that passes against base is not by itself proof that the PR test is inadequate.",
"Negative controls are only candidates until the test actually runs against base and fails by assertion rather than import/setup error."
]
}Precheck helper errors:
/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck-14072-current/head-4b3d7091e600-031cf366/tools/pr_abi_audit.py': [Errno 2] No such file or directory
/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck-14072-current/head-4b3d7091e600-031cf366/tools/pr_test_quality.py': [Errno 2] No such file or directory
Exact-head hosted checks (the cancelled conventions attempt is superseded by the successful run on the same head):
conventions 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed cancelled
conventions 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed success
eligibility 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed success
labels 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed success
library-portability 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed success
linux 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed skipped
relinker (macos-latest, clang++) 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed success
relinker (macos-latest, clang++, x86_64) 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed success
relinker (ubuntu-latest, g++) 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed success
relinker (windows-latest, g++) 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed success
relinker-asan 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed success
report 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed success
windows 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed success
write-watch 4b3d7091e60031ba0cd1635edb8f8ba5e6a3ee68 completed skipped
Negative control
| Assertion | Change to code under test | Did the new test fail? |
|---|---|---|
| Cross-lane reads on one-lane waves must throw, while supported own-lane reads remain accepted. | The PR reports removing each lane check or relaxing the selector-walk rules. | The PR reports the matching test fails; I did not run these mutations. |
Findings
Blocking
None.
Non-blocking
The Linux product job was skipped, and I did not independently reproduce the author-reported focused tests or mutation checks. The added domain-wave test is registered despite the heuristic warning.
Sources
- Cross-lane emitter implementation at the reviewed head
- One-lane domain-wave tests at the reviewed head
- Test registration at the reviewed head
- Linux/RADV paired validation report for the reviewed head
- Current-main math changes integrated by the merge preview
Not verified
I did not compile or run the CTest cases locally, nor test on a console. The Windows/macOS results and mutation checks are author-reported; the separate Linux/RADV results are from the linked PR comment and were not independently reproduced here. The clean merge preview against current main dc9dc7eaaeaf99b1c491d426e68bb999def3da16 was not built.
✅ 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 (3, not counted against this PR)Validation IDs already present on main (5 IDs in 480 tests)Skipped (44)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. |
|
Hi @frankea, the new
Please add a rejected |
…tive-lane address
|
@oneandonlydean thanks, both are pinned in 067a530. Guest code cannot write either address: under the shift the walk accepts only the lane id, an AND with it or the first-active-lane select, and guest code reaches the lane id only through Tested on 067a530: the 21 |
✅ NVIDIA GPU test: passedPR head
Failing on main too (3, not counted against this PR)Validation IDs already present on main (9 IDs in 585 tests)Skipped (51)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
Follow-up to #14068, which this branch sits on. On a device whose
subgroupSupportedStageslacks the vertex stage (lavapipe, MoltenVK), vertex and local programs run as one-lane waves (#4470): the invocation is lane 0, a ballot is its own bit and a shuffle returns its own value. DPP moves,ds_swizzle_b32,ds_bpermute_b32andv_permlane16_b32had no lane check on that path, so a value the console reads from another lane's VGPR came back as an inactive lane's (0, thebound_ctrlvalue or the old value) or, with fetch-inactive, as the invocation's own. #14068 listed them in the "One-lane vertex waves" debt entry as unchecked. Merged hull programs also run at one lane, butrequireOwnLanesinTessellationLowering.cpprefuses every cross-lane opcode before the SPIR-V emitter, so they never reach these checks.FailOutsideHostSubgroupwhen lane 0 reads another lane, for exampleDPP read of lane 1 is outside the 1-lane host subgroup that runs this wave64 program at one lane per invocation.EmitDppMoveU32). Lane 0's source comes from DPP_CTRL (RDNA 2 ISA, 13.3.9, Table 91, page 268):quad_permreadsdpp_cntl[1:0],row_shl:nlane n,row_ror:nlane 16 - n,row_mirrorlane 15 androw_half_mirrorlane 7.row_share:nandrow_xmask:n(0x150-0x16F, LLVM's dpp16_ctrl andDppCtrlinSIDefines.h) read lane n. DPP8 readsLANE_SEL0(13.3.10, Table 92, page 269).row_shrgives lane 0 no source ("else use bound_cntl"), so therow_shrsteps of the wave scans still build, the elected f32 maximum ofverifySingleLaneElectedAtomicMaxamong them.ds_swizzle_b32(EmitSwizzleU32). Lane 0's source comes from the offset (12.13.1, page 207):or_mask ^ xor_maskin the 32-thread mode,offset[1:0]in the 4-thread mode, and the rotate mode asEmitDsSwizzleTargetLanedecodes it.ds_bpermute_b32(EmitBpermuteU32). Lane 0 reads lane(ADDR + OFFSET) / 4(12.13, page 206) of its 32-lane half, as the emitter takes it. A constant address must name lane 0. Any other address throws unlessOwnLaneSelectorWalkfrom fix(shader): throw for v_readlane_b32 of a register-selected lane on one-lane waves #14068 proves it is the address the translator builds forimage_sample_*_cd, which takes its derivatives from the quad's first active lane:Select(mask != 0, quad + FindILsb32(mask), lane) << 2, wherelaneis the lane id,quadis an AND with it, andmaskis a word built from ballots (for it the walk also accepts a ballot word shifted right). On one lane that is the invocation's own lane, so these samples keep building. Guest code reaches the lane id only throughv_mbcnt, which lowers to aBitCountthe walk never accepts, so every register address a guest program writes throws.v_permlane16_b32(EmitPermlane16U32). Lane 0 readsS1[3:0](12.12, page 186,lanesel = {S2, S1}), so S1 must be a constant with those bits clear.v_permlanex16_b32was already checked.verifyNarrowVertexWavealready throws on one lane at itsv_permlanex16_b32and still does. The DPP reads in the vertex programs of the execution tests arerow_shrsteps andquad_perm:[0,1,2,3], and none of them usesds_swizzle_b32,ds_bpermute_b32orv_permlane16_b32, so lavapipe runs them as before.main, the merge also drops the tessellation evaluation entry's note that a register-selectedv_readlane_b32lane is not checked, which fix(shader): throw for v_readlane_b32 of a register-selected lane on one-lane waves #14068 makes untrue; that entry already defers to the one-lane vertex rules.main, puts tessellation evaluation programs without subgroups on the same path, so they get these checks with no further code; itsverifySingleLaneDomainWavepasses with them.Title: none. In my Gran Turismo 7 (PPSA01317) shader disk cache from MoltenVK runs on an Apple M5 Max (27,036 entries), the 1,198 distinct programs compiled at one lane per invocation (1,055 vertex, 137 tessellation evaluation, 6 local; the cache key records the host width) hold no DPP,
ds_swizzle_b32,ds_bpermute_b32,v_permlane16_b32orv_permlanex16_b32, so none of them starts throwing. They were disassembled with llvm-mc 18 as gfx1013, since gfx1030 lacks thev_mad_f32these programs use, and a raw scan of the DS and VOP3 opcodes agrees.Tested
Windows 11 on an RTX 4090 (driver 616.92, Vulkan 1.4.341), winlibs GCC 15.2.0, Release with
BUILD_TESTING=ON. The 4090 has vertex subgroups, so its draws keep the subgroup path; the one-lane path runs there only in theagc_shader_memory_testschecks, which compile for explicit targets.067a530fec: fix(shader): throw for v_readlane_b32 of a register-selected lane on one-lane waves #14068'sdcd3021feaplus this change) merged ontomained5c169: the 25 entries ofagc_shader_memory_tests,agc_driver_read_lane,agc_driver_write_lane,agc_driver_lds_mask_waterfall,agc_driver_exec_host_lanes,agc_driver_ngg_vertex_wave_reduction,agc_driver_pixel_wave_reduction,agc_driver_pixel_row_reduction,agc_driver_wave32_subgroup,agc_driver_window_lane_spill,agc_driver_scalar_sop1,agc_driver_tessellation,agc_driver_tessellation_depth_range_remap,agc_driver_relative_ancillary,agc_driver_image_sample_derivativesandagc_driver_vertex_elected_atomic_max(skipped: the 4090 has vertex subgroups). They pass once more underVK_LAYER_KHRONOS_validationwithANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1(ctest -V), with noVUID-lines and nowas not found but was requested by env var VK_INSTANCE_LAYERS; the loader log foragc_driver_read_lanein that environment shows the layer inserted. The same entries, then 36 (21 inagc_shader_memory_tests), passed the same way on the earlier heads2928046282,6617bb0427,4d7358d6ac,68c45dc452and4b3d7091e6and onmain029463c and e8c25dd.Apple M5 Max, macOS 27.0.1, x86-64 under Rosetta 2: the 21
agc_shader_memory_testsentries pass 3 times in a row on this branch (067a530fec), built natively. These compile for explicit targets and run no GPU work. One local build aid that is not part of this pull request was needed for the build: theGuestMemory.cpppart of #11447.New checks, in
verifySingleLaneCrossLaneReads(tests/ShaderMemory.cpp, beforeverifySingleLaneVertexWave), all in wave64 vertex programs:quad_perm:[0,0,2,2],row_shr:1,row_share:0anddpp8:[0,0,0,0,4,4,4,4],ds_swizzle_b32swizzle(BROADCAST,4,0),ds_bpermute_b32of address 0,v_permlane16_b32with selects 0x76543210, andimage_sample_cd.quad_perm:[1,0,3,2](lane 1),row_shl:1(1),row_ror:4(12),row_mirror(15),row_xmask:2withfi:1(2) anddpp8:[7,6,5,4,3,2,1,0](7),ds_swizzle_b32swizzle(SWAP,1)(1) andswizzle(QUAD_PERM,2,1,0,3)(2),ds_bpermute_b32of address 4 (1) and atoffset:4(1),ds_bpermute_b32(register-selected) of the vertex index, of thev_mbcntlane id plus the vertex index, of the vertex index plus 32, of the vertex index ORed with 32 and of the vertex index atoffset:128,v_permlane16_b32with selects 1 (1) and with a selectv_readfirstlane_b32took from the vertex index (register-selected).image_sample_*_cdaddress. So three programs are built as IR and emitted for a one-lane host subgroup: with that address theds_bpermute_b32builds, and with it shifted left by 3 instead of 2, or with the next quad's first active lane,(quad + 4) + FindILsb32(mask), it throws (register-selected).tools/check_conventions.py --base origin/mainpasses on the branch, and it merges cleanly withmained5c169.tools/pr_overlap.py --branch fix/one-lane-cross-lane-readsagainstmain0da8261 finds one overlap, #15573, which rewords the debt line next to the tessellation evaluation entry this branch edits; 41 pull requests that conflict withmainthemselves share files with this branch and could not be compared.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)