Repository navigation
Conversation
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the quick patch. The approach (a shared per-shader budget, charged by failed and successful walks) looks reasonable and the pass stays semantically safe, but the PR doesn't show yet that it fixes #3504:
- No before/after numbers:
0x200f780000/0x200f7a0000timings on this head are needed (the issue shows 146.7 s / 406 s onmain). Please post them fromagc_shader_replaywith the counters (walks, values visited, selects removed), or ask @ThatsLeo to run them in #3504. - The budget is
32 × walkLimit, picked without data. Please show how the time and the number of removed selects vary with it on those shaders (the issue reports 4,616 removed after fda4aa9 vs 1,391 before), and that the SPIR-V stays valid. A budget that is spent by the first masks leaves later selects in, so report that cost too. agc_masked_selectonly checks that the budget gets consumed. It can't fail for the wrong reason (it doesn't compile without the change). A case where a select provable within the budget is still removed after earlier walks have spent part of it would pin the "proof completes, select removed" half.
Happy to merge once the measurements are in.
|
Added commit The two Stray replay request files are not available in this workspace, so I can't provide the timing, select-count or SPIR-V results from here. I asked the issue reporter to run both shaders at 1x, 8x and 32x the per-walk limit, and included the requested output fields in the issue comment. The PR description records this as pending rather than claiming an unverified improvement. |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks for the follow-up test; agc_masked_select passes here on the PR merged onto current main (Linux, scoped build, 404/404 ctest in libSceAgc/libSceAgcDriver), and the new stats make the tests fail to build without the source change.
What is still missing is the one measurement the claim rests on, which we can't produce without the Stray requests:
0x200f780000/0x200f7a0000replayed on this head vsmain(time, values visited, selects removed, SPIR-V valid), as requested in #3504. If @ThatsLeo posts it there, it is enough.- The 32x multiple needs that data (or a stated reason), since a lower value than the shader needs leaves selects that other shaders (e.g. the ones in #3391) rely on being removed.
Once the numbers are in, this is ready to merge from our side.
|
Conflicts with main in ThatsLeo's tables in #3504 cover the numbers (141 s → 1.3 s and 385 s → 2.4 s at 32x, SPIR-V valid). They also show that with this PR alone So either this goes in after #4031 (list it under Depends on), or the multiple has to be one that keeps that shader compiling. |
|
Rebase handoff — this branch was ~800 commits behind The conflicts were semantic, not mechanical — Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release,
To pick it up (either works): Or cherry-pick the resolution from |
|
Closing this as superseded by merged #4331. Its backward-pass masked-lane analysis fixes the repeated per-select walk that motivated the shared budget and avoids the budget-dependent selects noted in #3504. The budget implementation is no longer needed on current main; #4331 documents the replay evidence and full-output behavior. |
What
Fixes #3504. Stray (PPSA02101) takes 146.7 seconds to prepare shader
0x200f780000on currentmain, with nearly all the time inMaskedSelectEliminator::Eliminate. InMaskedSelectEliminator.cpp,MaskedLaneWalk::UnobservedWhereMaskedresets its full program-scaled work limit for every candidate select. The issue's counters show more than 1.2 billion values visited across those repeated walks.Keep the per-walk limit, and add one shared work budget for the entire shader. Charge the budget on both successful and early-failed walks. Once the budget is consumed, later proofs return “not proven”; a select is removed only when the existing proof completes successfully. The pass can leave more selects in the IR, but does not change shader semantics. The result reports the analysis work and budget for regression coverage.
The
agc_masked_selectregressions build a long chain of predicated selects, verify the analysis stops at its per-shader budget and keeps unresolved selects, and verify that a later select is still removed after an earlier masked proof spends part of the budget and conservatively fails.#3391 changes the same pass to prove whole-quad mask implications for GTA V. Its behavior is separate from this per-shader work budget; this PR does not depend on it. The overlap is limited to the shared optimizer and test files, and may require a conflict resolution if either branch merges first.
I could not replay Stray's dumped shaders locally. The reported 146.7-second main result and counters are from the issue report. I asked the issue reporter to compare the two shader requests at 1x, 8x and 32x the per-walk limit, including SPIR-V validity; those title-specific measurements and budget tuning remain pending.
Tested
agc_masked_select_tests.agc_masked_select— passed three consecutive runs.python3 tools/check_conventions.py --base upstream/main— passed.git diff --check— passed.Checklist
main; no open PR implements this per-shader budget behavior