Repository navigation
fix(stinkytofu): count a ds credit wait as elapsed time; merge CDNA5 DAG Phases C and D - #14805
Merged
Merged
Conversation
…Fill pick Phase C (inside a WMMA window) and Phase D (outside) ran the same pick and the same wait handling; only Phase C advanced time on a miss. Fold them into one pick that, inside a window with nothing pickable, advances to the oldest queued WMMA's end and picks once more. No behaviour change. The "Phase C / D" row of the pick table in the DAG AGENTS.md now describes the single phase. Checked: gfx1250 asm identical to develop on all 21 perf yamls (maf/mab/medium), StinkyTofu ctest 1826/1826.
A ds_load that waited for a free credit used to charge the wait to a separate scheduling
budget (dsSchedulingBudgetUsed_) and leave the clock, the credit queue and the queued WMMA
cover untouched. Inside a WMMA window that over-charged the next ds_load (the queue never
aged), and outside it the wait was not counted at all, so queuedCoverCycles() could be
overstated.
Every wait (ds credit, RAW, hazard) now advances the timeline in the one NonWmmaFill rule:
the credit queue ages and the cover shrinks by the wait. The ds fit check reads
coIssueCyclePos_ directly; dsSchedulingBudgetUsed_ and chargeDsCreditWait are removed.
This is a behaviour change, not a refactor: the generated asm differs from the previous
commit on 16 of the 21 gfx1250 perf yamls (all MAF yamls included). The barrier, signal and
tensor_load positions in the main loop are unchanged on the MAF yamls and on mxf8_tn_medium
and bbs_tn_medium; f8_tn_medium moves its barriers by 4 to 5 WMMAs and mxf4_tn_medium its
first barrier by 2.
WmmaRefill_{FixedBatch,Dynamic} expected shapes change from WddWWWWWWWWWdWdWdddd to
WddWWWWWWWWWddWWdddd: after a credit wait the queue has aged, so the next load needs no
refill WMMA in front of it.
ctest: 1842 of 1842.
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #14805 +/- ##
===========================================
- Coverage 77.35% 77.33% -0.02%
===========================================
Files 2894 2894
Lines 487162 487192 +30
Branches 73731 73720 -11
===========================================
- Hits 376823 376767 -56
- Misses 85448 85540 +92
+ Partials 24891 24885 -6
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
1 task done
geotseng-amd
approved these changes
Nov 1, 2026
KKyang
added a commit
that referenced
this pull request
Nov 2, 2026
…s pacer's line (#14813) Refs #14730 Stacked on #14805 (base branch `users/kkyang/stinkytofu-time-model`); review that one first. This PR's own diff is the two commits after it. ## Motivation A ds_load's right to issue in the gfx1250 DAG scheduler was assembled inline from three pieces: the mode2 WAR gate (a heuristic that holds a load back until enough matrix ops have issued after its WMMA reader), the overlap check against the queued WMMAs' sources, and a "relief" that lifts the gate when the region is behind its ds rate. The relief used a second, region-wide rate (`expectedDs = dsTotal * wmmaIssued / wmmaTotal`) that the ds pacer does not share: the pacer plans by stage and batch, so the two could disagree about whether a load is due. ## Technical Details Two commits. - **`refactor`: one predicate for the ds WAR gate.** `warGateRelieved()` and `dsWarClear(ds)` name the checks; the candidate test is `pickedDS && dsWarClear(pickedDS)`. Same checks in the same order. The generated gfx1250 asm is identical to the previous commit on all 21 perf yamls. - **`fix`: lift the gate from the ds pacer's own line.** `DsSpreadPacer::behind()` is the number of loads the even line has placed by the current batch minus the loads the stage has issued; the gate is relieved when it is positive. Without a pacer plan there is no relief. `expectedDs` is removed. This is a behaviour change, not a refactor: the asm differs from the previous commit on 8 of the 21 gfx1250 perf yamls (`bbs_tn_m64_mab`, `bbs_tn_mab`, `bbs_tn_medium`, `f8_tn_mab`, `mxf4_tn_mab`, `mxf8_tn_mab`, `nvf4_tn_mab`, `nvf4_tn_maf`); the other 13 are identical. - **Tests.** `relief_releases_a_region_behind_its_ds_rate` now follows the pacer's batches (two loads after four WMMAs, one after eight, the last when WMMA #1's gap closes at ten). `stamps_survive_a_wmma_free_region` moves to its own file, `dag_mode2_war_gate_stamps.stir`, and runs with `--no-ds-spread`: with the pacer, the relief lifts the gate after the first batch and hides what that case checks (gate stamps survive a region cut). Its assertions are unchanged. - **Not in this PR.** #14730 also asks for one model of the hazard across the pacer's release time, the pipe-op gate and the queued-WMMA overlap. The pacer's release time (`earliestAt`) is a planning input and not a gate, the pipe-op gate is a tested heuristic that measurably helps, and the overlap check is the hardware truth, so they are not merged here. ## Test Plan StinkyTofu unit and FileCheck suites. Asm comparison against the previous commit on the 21 gfx1250 perf yamls (maf, mab, medium). Hardware A/B against the previous commit on every yaml whose asm differs, with interleaved runs in both orders and output validation on. ## Test Result Unit and FileCheck tests: 1846 of 1846 pass. Performance results are recorded in the JIRA ticket for this work, not in this description. ## Status Draft. The description matches the code on the branch. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests. --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
Contributor
TheRock Submodule Bump ActivityNewest first
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #14731
Motivation
In the gfx1250 DAG scheduler a ds_load that waits for a free credit charged the wait to a separate scheduling budget and left the clock, the credit queue and the queued WMMA cover untouched. Inside a WMMA window that over-charged the next ds_load (the queue never aged); outside a window the wait was not counted at all, so
queuedCoverCycles()could be overstated. The two cases were also handled by two phases (C inside the window, D outside) that ran the same pick and the same wait handling.Technical Details
Two commits.
refactor: merge Phases C and D into oneNonWmmaFillpick. Only Phase C advanced time when nothing was pickable. The merged rule picks once and, inside a window with nothing pickable, advances to the oldest queued WMMA's end and picks once more. The generated gfx1250 asm is identical to develop on all 21 perf yamls. The "Phase C / D" row of the pick table in the DAGAGENTS.mdnow describes the single phase.fix: a ds credit wait is elapsed time. Every wait (ds credit, RAW, hazard) advances the timeline, so the credit queue ages and the cover shrinks by the wait. The ds fit check readscoIssueCyclePos_directly.dsSchedulingBudgetUsed_andchargeDsCreditWaitare removed. This is a behaviour change, not a refactor: the asm differs from the previous commit on 16 of the 21 gfx1250 perf yamls, all MAF yamls included. The barrier, signal andtensor_loadpositions in the main loop are unchanged on the MAF yamls,mxf8_tn_mediumandbbs_tn_medium;f8_tn_mediummoves its barriers by 4 to 5 WMMAs andmxf4_tn_mediumits first barrier by 2.WmmaRefill_FixedBatchIssuesBatchSizeWmmasPerRefillandWmmaRefill_DynamicFillsToTargetexpectWddWWWWWWWWWddWWddddinstead ofWddWWWWWWWWWdWdWdddd: after a credit wait the queue has aged, so the next load needs no refill WMMA in front of it. The counts of loads and WMMAs are unchanged.*_finetune*yamls carry ds knob values tuned against the old accounting and may need a retune.Test Plan
StinkyTofu unit and FileCheck suites. Asm comparison against develop on the 21 gfx1250 perf yamls (maf, mab, medium). Hardware A/B against develop on every yaml whose asm differs, with interleaved develop/branch runs in both orders and output validation on.
Test Result
Unit tests: 1842 of 1842 pass. Performance results are recorded in the JIRA ticket for this work, not in this description.
Status
Draft. The description matches the code on the branch.
Submission Checklist