Repository navigation
refactor(shader): walk each construct when checking the structured constructs - #9703
Conversation
…f scanning them Merge splitting re-examines every selection header after each split, so the lookups behind each examination run millions of times on a large graph. On Ghostrunner's compute shader 0x200c520000 (4,750 blocks growing to 10,170 over 1,302 splits) agc_shader_replay spent 173 s in splitSharedMergeBlocks. A profile put most of it in linear membership tests and in FindNearestCommonPostDominator, not in the analyses. Loop bodies, construct blocks, selection regions and strongly connected components are sorted, so membership in them is now a binary search. The traversals that tested membership in the list they were building (selectionRegion, reachableWithin, naturalLoopBody) and externalEntryEdges test a bitset instead, and selectionRegion reads its sorted result from the bitset. dominatedBlocks tests the header once and pushes a successor only before it is visited. FindNearestCommonPostDominator skips a candidate whose post-dominator set is smaller than the other common post-dominators and checks the rest in one pass over the set. remapIds sorts only when the map broke the order. Each change keeps the same sets in the same order and the same first candidate, so the structured graph is unchanged. On an M4 Pro the shader's structurization takes 37 s instead of 176 s.
…rating dominator sets recomputeAnalyses runs after every merge split, and its dominator and post-dominator fixpoints intersected a bitset of every block for every block until nothing changed. They now build the tree with the Cooper-Harvey-Kennedy iteration over a reverse post-order from a virtual root above the entry block, or above every exit for post-dominators, and write each block's sorted set as its parent's set plus the block itself. The sets are the same. When every block is reachable from the virtual root, the greatest fixpoint of the old equations is the dominance relation of that rooted graph, which the tree describes. When a block is unreachable from the entry, or cannot reach an exit, the old fixpoint runs as before. The post-dominator tree also keeps each block's immediate post-dominator, and FindNearestCommonPostDominator climbs it when both blocks have one: the sets are the tree's paths, so the deepest common block is the one the set intersection found. Blocks added or cloned after the analysis have no parent and take the set path, as before. APS5_NO_PERF_DOMINATOR_TREE turns both off, and the structurizer and control-flow tests also run with it set. On an M4 Pro the structurization of Ghostrunner's compute shader 0x200c520000 takes 18 s instead of 37 s.
computeStronglyConnectedComponents visited blocks recursively, one stack frame per block on the deepest path. The reach-flag fallback turns Ghostrunner's compute shader 0x200c520000 into a graph of 32,490 blocks, and with APS5_FORCE_STRUCTURIZER_FALLBACK the visit overflowed agc_shader_replay's 2 MiB stack after 4 s. The visit now keeps its frames in a vector. It enters successors in the same order and closes components in the same order, so the components, their entry blocks and the irreducibility verdict are unchanged.
After each split, every selection header that needs no work still walks its construct (dominatedBlocks) to learn which predecessors of its merge lie inside. On Ghostrunner's compute shader 0x200c520000 that is 1.7 million walks. When every block is reachable from the entry and the predecessor lists match the successor lists, and the merge M neither is the header H nor dominates it, the walk finds exactly the blocks that H dominates and M does not, other than M. A block the walk reaches is dominated by H and reached from the entry on a path that avoids M, because some path to H avoids M. A block that H dominates and M does not lies on a path from the entry that avoids M, and from the last H on that path every block is dominated by H, so the walk reaches it. splitOneSelectionMerge checks those conditions once per pass and then asks the dominator sets directly; otherwise it walks as before. Only membership in the construct is used, so the split and its order are unchanged. On an M4 Pro the shader's structurization takes 12 to 15 s instead of 18 s, and 176 s before this series.
…ucts hasStructuredConstructs tested every block of the graph against each merge header, and searched every block for a block's innermost loop, with dominator binary searches each time. Under APS5_FORCE_STRUCTURIZER_FALLBACK, Ghostrunner's compute shader 0x200c520000 becomes a 32,490-block reach-flag graph with 6,647 merge headers, and the check took 44 to 50 s of the 49 to 55 s. The innermost loop is now searched among the loop headers only. Every other block failed the loop-header test first, so the candidates and their order are unchanged. When constructsFollowDominance holds for the structural graph, the member checks visit only the blocks dominatedBlocks finds from the header, stopping at its merge, and for a loop also from its continue block, in block order. Only a block in the header's construct or in the loop's continue construct can fail a member check. A construct block is dominated by the header and not by the merge, so some path from the entry reaches it without the merge, and from the last header on that path every block is dominated by the header, so the walk reaches it. A continue-construct block is dominated by the continue block and is reached from it the same way. The walk finds every such block, and the blocks it skips passed every check, so the result is unchanged. Otherwise every block is checked as before, and APS5_NO_PERF_DOMINATOR_TREE keeps the old loop. On an M4 Pro the check takes 0.27 s instead of 44 to 50 s on that graph, and the forced fallback takes 5.1 to 5.4 s instead of 49 to 55 s. The default structurization of the shader makes one check, which takes 0.03 s instead of 0.36 s.
github-actions
Bot
added
perf
|
This pull request is labeled |
|
Tested on AMD Ryzen 7 9800X3D and AMD Radeon RX 7900 XTX (Fedora Linux, Mesa 26.2.5):
|
|
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
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the construct-walk optimization against the previous hasStructuredConstructs logic, including the structural graph analysis, the loop/continue member sets, and the fallback path. The optimized walk is guarded by constructsFollowDominance, which checks reachability and predecessor/successor consistency; the original full scan remains as fallback. The reported differential comparisons and repeated focused/Vulkan-validation runs are meaningful evidence. I found no independent code defect in the reviewed diff. I could not run the C++ suite locally because this checkout has no configured product build.
Before approval, please satisfy the repository performance-report rule: this PR is labeled perf, but the description does not include matching aps5-summary.jsonl lines for main and the change from the same title, scene, and machine, the two trace attachments, or tools/trace_digest.py output. The workflow has marked perf numbers missing. Please add the capture artifacts, or remove the performance claim/title if the required capture is unavailable.
This branch is based on 7e2ba73e4c, 13 commits behind current main (8b3a6ba88d8fc74e768f1a7e8e9e36fb69842768), and depends on #9702, which has the same base age and needs the same performance-report artifacts. Please keep the declared dependency, coordinate the stack after #9702 is updated, and rebase/retest this head on current main before requesting another review.
Local checks I ran against the head: git diff --check passed; tools/check_conventions.py passed with the actual PR body. GitHub conventions, eligibility, intake, labels, and report checks pass; product build jobs are skipped by the repository gates. I did not independently reproduce the reported timings, differential fuzzing, or platform/GPU test results.
frankea
changed the title
github-actions
Bot
removed
perf
|
@MarcZX10 thank you for the review. The stack now merges No scene can give the capture: the only graph where this check is large is Ghostrunner's 0x200c520000 under the forced fallback, and that scene ends before its next present. So I've taken the other route you offered. The title is now AI-assisted: yes (Claude Code) |
|
@MarcZX10 since your review this PR is retitled as a refactor with no speed claim (the perf labels came off), and it now merges #9702's refreshed head on AI-assisted: yes (Claude Code) |
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Checked the construct walk in |
What
hasStructuredConstructsruns after every structurize attempt. For each merge header it tested every block of the graph, and for each block it searched every block for its innermost loop, with dominator binary searches each time. The work grows with the number of headers times the number of blocks. WithAPS5_FORCE_STRUCTURIZER_FALLBACK=1, Ghostrunner's compute shader 0x200c520000 (PPSA03685, linked with #9068 and #8070) becomes a 32,490-block reach-flag graph with 6,647 merge headers, so the check runs about 216 million member tests.constructsFollowDominance(from fix(shader): structurize a 4,750-block graph before Unreal's render thread watchdog fires #9702) holds for the structural graph, the member checks visit only the blocksdominatedBlocksfinds from the header, stopping at its merge, and for a loop also from its continue block, in block order. Only a block in the header's construct or in the loop's continue construct can fail a member check. A construct block is dominated by the header and not by the merge, so some path from the entry reaches it without the merge; from the last header on that path every block is dominated by the header, so the walk reaches it. A continue-construct block is dominated by the continue block and is reached from it the same way. The blocks the walk skips passed every check, so the result is unchanged.APS5_NO_PERF_DOMINATOR_TREEkeeps the old loop. Its row indocs/dev/ENVIRONMENT.mdsays so.Tested
main0db47b2 (head 98c6aef, through fix(shader): structurize a 4,750-block graph before Unreal's render thread watchdog fires #9702's refreshed head; no conflicts), RTX 4090 (NVIDIA 616.92, Windows 11): the 11 ctest entries ofagc_structurizer,agc_shader_control_flow,scalar_callandagc_driver_compute_table_callspass 11/11, each test run three times, and 11/11 under the validation layer (VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation,ANYPS5_NO_SHADER_CACHE=1,ANYPS5_REQUIRE_VULKAN=1,ctest -V), with noVUID-line; the layer reports nothing on this head.main0db47b2 passes the union of the refreshed sets, 115/115, under the same layer settings with noVUID-line.This PR makes no speed claim. The repository's speed evidence is a capture of a scene on
mainand on the change, and no scene runs this check on a large graph: the only one known is 0x200c520000 under the forced fallback, and its scene ends before its next present (fix(shader): structurize a 4,750-block graph before Unreal's render thread watchdog fires #9702 explains why).Same result:
APS5_NO_PERF_DOM_INTERSECTION,APS5_NO_PERF_ITERATION_SELECTION,APS5_FORCE_STRUCTURIZER_FALLBACKandAPS5_NO_PERF_DOMINATOR_TREE: 250,000 comparisons ofGraphToString(or the exception text), no differences.Tests on the head, 6ba7289 on
main7e2ba73 (on top of #9702), Windows 11, RTX 4090, NVIDIA 616.92, WinLibs GCC 15.2:agc_structurizer,agc_shader_control_flow,agc_driver_compute_table_callsandscalar_callpass 11/11 with theintersection_off,iteration_selection_off,dominator_tree_offandforced_fallbackvariants, each run three times (ctest --repeat until-fail:3).Validation: I ran the same focused tests on this head with
VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(Vulkan SDK 1.4.363.0),ANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1, usingctest -R "^(agc_structurizer|agc_shader_control_flow|scalar_call|agc_driver_compute_table_calls)" -V. They pass 11/11. The log has nowas not found but was requested by env var VK_INSTANCE_LAYERS, noVUID-line and no validation message.After the
mainmerge, Windows 11, RTX 4090, NVIDIA 616.92, WinLibs GCC 15.2: 958f3f9 mergesmain7b32a84, through #9702's merge.agc_structurizer,agc_shader_control_flow,agc_driver_compute_table_callsandscalar_callpass 11/11 with theintersection_off,iteration_selection_off,dominator_tree_offandforced_fallbackvariants, each run three times (ctest --repeat until-fail:3). UnderVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(Vulkan SDK 1.4.363.0),ANYPS5_NO_SHADER_CACHE=1andANYPS5_REQUIRE_VULKAN=1, withctest -V, they pass 11/11; onmain4d57821, five commits past the mergedmainand none incore/shader, the nine of them that exist there pass 9/9 (the twodominator_tree_offvariants come with #9702). Neither log haswas not found but was requested by env var VK_INSTANCE_LAYERS, aVUID-line or any other validation message.Checklist
main; no other open PR implements the same functions. No open PR changesStructurizer.cpp.NotImplemented_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)