Repository navigation
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I found a safety-check bypass in analyze(): the workgroup size <= 32 fast path returns eligible before scanning instructions or applying the later guards. In particular, it skips the explicit barrier && execBranches rejection (and the tests only cover that rejection with the default 16x16 workgroup). A wave64 compute program with 32 or fewer work items, a barrier, and an ExecZero/ExecNonZero branch is therefore emitted with laneCount = 1 despite the analysis explicitly rejecting barriers in programs with exec branches. That can put a workgroup barrier under non-uniform control flow and hang or invalidate the dispatch. Please retain the simple one-to-one lane mapping optimization for small groups, but still run the structural checks that apply to them; add a <=32-work-item regression containing an exec branch and barrier and require the analysis to reject it.
I have not approved this shader change. The current report has extensive tests and a same-scene performance pair, but that does not cover this early-return path.
|
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 The early return is deliberate. A workgroup of at most 32 work items fits in one 32-lane host subgroup at either layout, the same subgroup that holds it at two lanes per invocation on |
|
@meh7an thanks, the proof is carefully built and the negatives are good. There is one case I don't think it covers yet: scalar values that leave a region one half of the wave can skip. At one lane per invocation, each 32-lane host subgroup decides Hardware: every lane gets 1. Single-lane: lanes 32-63 get the old s2. As far as I can tell, every rule passes here: there is no barrier, the saved exec is a symmetric ballot pair combined bit by bit, and Compiler output normally sends such live-outs through VGPRs, but the analysis claims a proof. Could it reject this shape? One option: at the merge of an exec/VCC branch, or at the exit of a loop whose back edge is an exec/VCC branch, reject any phi whose incoming value from the region isn't exec-selected (or isn't identical on both paths). A negative case for the snippet above in A smaller question: the first commit's One more question: |
|
@oneandonlydean thanks, all three hold up.
These can change which programs qualify, so I'll re-measure the S3K pair and update the numbers with the push. |
adriadam10
left a comment
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the careful proof and the negative cases. The head is still 5d770a5ff0, and the three gaps raised in this thread (which you confirmed) are open, so this can't merge yet:
SingleLaneAnalysis.cpp:925: the<= 32early return skips every check, includingbarrier && execBranches(line 984). Please run the structural checks for small groups too, and add a <=32-thread regression with an exec branch and a barrier that must be rejected (@MarcZX10's request).- Same early return: whole-wave mode (
s_or_saveexec_b64 -1) wakes lanes 32-63, which exist only in the second slot at two lanes. Drop the shortcut and add the 32-thread group reading lane 63 in whole-wave mode, as you proposed. - Scalar/VCC/goto-flag/window-store values defined in a region that one half can skip via
execz/execnz/exec-implied VCC reach the merge phi (or loop exit) with different values per half.Uniformitytreats those phis as uniform. Please add the live-out rule you described, with the negatives (@oneandonlydean's snippet, anexecz-exited loop with a scalar counter, a VCC written in the region and read after it, a whole-wave write in the region) and the exec-masked VGPR positives. - The first commit makes a constant-true lane bit count as every lane in
MatchHalfWaveScan/MatchRowScanfor two-lane programs too. Please add the wave64 compute GFX10 reduction unders_or_saveexec_b64 -1on a 32-lane target that you described.
Please also refresh the S3K numbers after the push, since the live-out rule may change which programs qualify.
Fold And32, Or32 and Xor32 of a value with itself, with its complement and with an absorbing constant, bit 0 of all ones shifted right by an amount masked to 0-31, and 0 shifted right by any amount. The lane masks the translator builds reduce this way: s_orn2_saveexec_b64 of exec gives exec | ~exec, all ones, so its lane bits and the exec selects they guard fold away. The half-wave scan matcher (2489437) took a constant-true lane bit only on hosts narrower than the wave and otherwise required the exec | ~exec structure, which no longer survives the fold. A constant-true bit now counts as every lane on every host, the same proof as x | ~x, so EveryLaneBit and its narrow-only flag go.
On a 32-lane host a wave64 compute program runs two guest lanes per invocation, so each invocation does every ALU operation, load and sample twice. SingleLaneAnalysis proves on the IR that the program gives every lane the same results at one guest lane per invocation, where each host subgroup holds half a wave, and the emitter then lays it out that way. A program qualifies when: - no lane reads another lane's value by index: readlane and readfirstlane read only uniform values, and there are no permutes, DPP, swizzles or GPU-selected buffer descriptors; - lane masks are combined bit by bit and read only as each lane's own bit, with the same expression over both 32-bit halves; - LDS is lane-private: every access is one affine function of the local invocation id and no two lanes' footprints share a byte, and a one-wave workgroup does not write it; - each term of a VCC branch's predicate is uniform or implied by the exec mask at the branch; - nothing leaves a region that one half of the wave can skip unless it is the same whether that half ran the region or not; - no barrier shares the program with a branch one half can decide alone. A half decides an exec branch, and a VCC branch on lane-varying terms, by itself. For each such branch the skipped region is what the taken side reaches before the immediate post-dominator and the other side does not. A value leaving it qualifies when it is a VGPR written under the branch's exec, a lane mask word that keeps its bits outside that exec, or a vector window store that keeps the old value outside it; scalar, VCC and goto flag values written in the region reject. A value no instruction reads, such as a phi cycle that only feeds itself, leaves nothing to compare. A memory write in the region needs a lane predicate inside the branch's exec. A loop left through such a branch must carry values that stay the same per lane and an exec that only narrows. Workgroups of at most 32 lanes get the same checks, since whole-wave mode still decides branches per half. The region check runs in the first simplify pass, before the vector window store and masked select eliminators drop exec selects whose inactive lanes nothing reads, and the program metadata keeps its verdict for the layout. Relatively addressed VGPRs live in a function-scope window per lane (LoadVectorWindow, StoreVectorWindow), so window accesses are lane-local; a window load counts as lane-varying, so readfirstlane of one keeps two lanes. BlockInfo::exec records the exec mask at each VCC and exec branch; the IR reclaim, identity resolution and redundant value elimination keep it current. A proven program needs no spanned layout, so APS5_SPANNED_WAVE applies only to programs the analysis rejects. The disk cache key marks the requests the rule lays out, so a run with APS5_NO_SINGLE_LANE_RULE=1 does not load their results. APS5_TRACE_SINGLE_LANE=1 prints each program's verdict and the reason a program keeps two lanes.
5d770a5 to
7a8da11
Compare
github-actions
Bot
added
the
perf numbers missing
|
This pull request is labeled |
|
@oneandonlydean @adriadam10 updated in 7a8da11, on
Re-measure. With the sound rule, S3K's heaviest compute (16x16 threads, about 5.4 ms per present) keeps two lanes, and rightly. Its loop at guest |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — prior finding resolved
I inspected the current head 7a8da113db96ef882b362d52af10e10ee08a9773. AnalyzeSingleLane no longer has the small-workgroup early return; it runs the branch/uniformity checks and rejects barrier && execBranches before returning eligibility. The added 32-thread barrier/exec-branch case directly covers the scenario from my earlier review, so I withdraw that blocker.
The author also reports adding live-out checks and regressions for the scalar/VCC/goto/window/loop cases raised by the other reviewer. I have not independently audited that new analysis or run its shader tests in this pass, so this comment is not an approval of the whole compiler change. The author has kept the PR in draft because the revised S3K comparison is flat and the large workload remains on two lanes; the description makes no current speedup claim. That seems appropriate while there is no measured benefit to support a performance PR.
What
Draft. With the sound proof below, no program on the measured S3K page that takes more than 1 ms qualifies, so the change shows no gain there. It stays a draft until a heavy compute program in some title qualifies.
On a 32-lane host,
mainruns a wave64 compute program at two guest lanes per invocation (laneCount2), so each invocation does every ALU operation, load and sample twice. This PR proves, on the IR, which wave64 compute programs give every lane the same results at one guest lane per invocation, where each host subgroup holds half a wave, and emits those programs that way. The emitter already supports this layout:APS5_SINGLE_LANE=<addresses>forces it by hand. Two commits:perf(shader): fold bitwise self and complement identities(ConstantFolder). It foldsAnd32/Or32/Xor32of a value with itself, with its complement in either order, and with an absorbing constant (x & 0,x | ~0). It also folds bit 0 of all ones shifted right by an amount masked to 0-31 to one, and0 >> yto zero.s_orn2_saveexec_b64of exec givesexec | ~exec, so its lane bits and the exec selects they guard fold away. The half-wave scan matcher (2489437) proved "every lane" only from theexec | ~execstructure on hosts as wide as the wave, which no longer survives the fold. A constant-true lane bit now counts as every lane on every host, the same proof, soEveryLaneBitand its narrow-only flag go. On its own this gave no measurable gain; it simplifies the lane masks the second commit reads.perf(shader): run wave64 compute at one lane where no lane can tell.SingleLaneAnalysisdecides before emission. A program qualifies when:readlaneandreadfirstlaneread only uniform values, and there are no permutes, DPP, swizzles or GPU-selected buffer descriptors;Skipped regions. At one lane,
EmitWaveMaskBranchConditiondecidesexecz,execnz,vcczandvccnzper host subgroup, so a half with no active lane skips what the whole wave runs. For each exec branch, and each VCC branch on lane-varying terms, the region is what the taken side reaches before the branch's immediate post-dominator and the other side does not. A value leaving it qualifies when it is a VGPR written under the branch's exec, a lane-mask word that keeps its bits outside that exec, or a vector-window store that keeps the old value outside it. Scalars, VCC and goto flags written in the region keep two lanes. A value no instruction reads, such as a phi cycle that only feeds itself, leaves nothing to compare. A memory write in the region needs a lane predicate inside the branch's exec. A loop left through such a branch must carry values that stay the same per lane and an exec that only narrows. Workgroups of at most 32 lanes get every check, since whole-wave mode still decides branches per half. The check runs in the first simplify pass, beforeVectorWindowStoreEliminatorandMaskedSelectEliminatordrop the exec selects whose inactive lanes nothing reads, andIrProgramMetadatakeeps its verdict for the layout.Relatively addressed VGPRs live in a function-scope window per lane (
LoadVectorWindow/StoreVectorWindow, f8ed012), so window accesses are lane-local; a window load counts as lane-varying, soreadfirstlaneof one still keeps two lanes. Only hosts whose wave64 compute target is 32 lanes wide are candidates. A qualifying program is emitted with a 64-lanehostSubgroupSize, asAPS5_SINGLE_LANEdoes, and the artifact records it;computeSubgroupSizethen still requires 32-lane subgroups where the device can.BlockInfo::execrecords the exec mask at each VCC and exec branch.ReclaimUnusedValues,ResolveControlFlowIdentitiesandRedundantValueEliminatorkeep it current, as they docondition.APS5_NO_SINGLE_LANE_RULE=1, which keeps every program at two lanes, does not load their results.APS5_TRACE_SINGLE_LANE=1prints each program's verdict and the reason a program keeps two lanes. Both are indocs/dev/ENVIRONMENT.md.agc_shader_memorychecks the two-lane SPIR-V of 64-thread programs, so it setsAPS5_NO_SINGLE_LANE_RULE.Correction to the first version. The first push admitted S3K's heaviest compute program and measured it at 4.87 to 2.82 ms per present. That admission was wrong. The program's loop at guest
0x2bfcis a per-lane state machine inv32. Its exit test runs onalive & state == 0, but the body then restores exec to every live lane and runs the state-2 handler (buffer loads, lane kills) for lanes still in state 2. A half with no state-0 lane would leave the loop at one lane while the whole wave keeps running that half's state-2 lanes. The program depends on the other half and keeps two lanes now.Relation to #5405, #7922 and #8004. Spanned waves (
APS5_SPANNED_WAVE) cover exactly-64-thread workgroups whose lanes exchange values, through workgroup barriers. This proof needs no exchange and covers any workgroup size, including multi-wave groups, which never span. A proven 64-thread program takes one lane without spanning; a program the proof rejects still spans when spanning is on, with or without #7922's default. Neither approach covers the other.Tested
Windows 11 (10.0.26300), NVIDIA GeForce RTX 2080 Ti, driver 617.14, i9-9900K, GCC 15.2 (WinLibs), Release. Base:
mainf8f5b24; this PR's head is 7a8da11, with no other PR applied.agc_single_lane(35 cases), run twice by ctest:agc_single_laneandagc_single_lane_off, the second withAPS5_NO_SINGLE_LANE_RULE=1, where its end-to-endRecompilecase expects a 32-lane layout instead of 64.readfirstlaneof user data, a vector window indexed by user data, a VCC branch on a uniform compare under the branch's exec, an exec-masked VGPR and a kept-value window store unders_cbranch_execz, anexeczloop carrying an exec-masked VGPR out, an exec-masked VGPR carried through a loop inside a skippable region, and a scalar written in a skippable region that nothing reads;readfirstlaneof a local id or of a window load, a VCC branch on a lane compare or on bits from a narrower exec, a VCC branch without its exec, DPP, lane 63 read in whole-wave mode, a barrier beside exec branches (also in a 32-thread group and beside a VCC branch on exec-implied bits), a scalar, a VCC word, a whole-wave VGPR write and an unmasked window store leaving a skippable region, a scalar loop counter used after anexeczloop, and a loop whose exit test reads fewer lanes than its body runs;~exec | execfolds to its true arm;agc_bitwise_identities(8 cases) covers the first commit.agc_shader_memory: a wave64 compute UMin scan unders_or_saveexec_b64 -1on a 32-lane target stays at two lanes and emits noOpGroupNonUniformUMin, since the scan matchers run only for non-compute stages at one lane (SpirvAluEmitterMath.cpp:1151). Next to it, the pixel version on 64 lanes still reads lanes 31 and 63 as two half-wave reductions.cteston this PR: 2840 of 2840 pass.guest_kernel_sys_threads_main,guest_kernel_sys_threads_main_patchedandnet_ipv6only_patchedwere left out: onmainthey stop with Windows' "Entry Point Not Found" dialog, which test(libs): give the prx loader tests the MinGW runtime on their PATH #12426 fixes.ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validationthroughctest --test-dir build -R <pattern> -V: the lane, exec and scalar-mask tests, the folder tests, the three new targets, and the 85 driver tests that took one-lane programs under the first version. All 110 pass on the change, and the 107 of them that exist onmainf8f5b24 pass there. Neither log has aVUID-error or awas not found but was requested by env var VK_INSTANCE_LAYERSline.APS5_DUMP_SHADERS=1and replayed throughagc_shader_replaywithAPS5_TRACE_SINGLE_LANE=1. Five run at one lane; of these only0x20092a600runs on every page frame, and it is small. Four keep two lanes: the 16-thread writer (a lane mask whose halves differ), a 512-thread program with a VCC branch on lane-varying bits, one withFPInterpolateF16, and the heaviest one (16x16 threads), whose state-machine loop the correction above describes.APS5_PROFILE_GPU=1: one boot per arm, warm caches, 12 page readings,[gputime]blocks 4 onward (9 onmain, 8 on this PR):main0x20092a600, 5 dispatches (one lane in this PR)No change beyond block-to-block noise;
main's first block (74.0 ms) is its own outlier. With no speed claim, I took one pair and no traces. The traces attached to the first version measured the wrong admission and no longer apply.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)