Repository navigation
[rocjitsu] Check wait dependencies once before instruction execution - #13019
Merged
Merged
Conversation
github-actions
Bot
added
documentation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
newling
self-requested a review
October 7, 2026 19:02
atgutier
approved these changes
Oct 7, 2026
Contributor
|
I previously considered a similar approach for the race detector plugin, i.e. instead of observing every individual register access inside the instruction’s emulation code, determine its register reads and writes from the decoded instruction’s operand and access metadata, and process that information once when the instruction is issued. For whatever reason, it got too complex and didn't work. I'd like to reconsider it though, it'd be great if some of the code introduced here can be reused for that. (agent review coming soon) |
Keep ordinary memory-wait diagnostics off by default and expose gfx1250 XCNT checking as a separate opt-in. This prepares the implementation for a separate decision about enabling ordinary checking by default. Develop already has a core per-wave logical scoreboard and a register shadow. Its enabled path checks through scalar/VGPR accessors during execution, with relaxed atomic shadow loads, thread-local scopes and observer suppression. These checks can repeat inside lane and matrix execution loops; producer footprints can depend on executed payloads. Move checking and producer registration to the instruction issuer before execution or async publication. Common instructions probe construction- time decoded register identities and widths, adjusted by the wave's current bank state. Reuse the 1280-byte shadow as an issuer-only byte array; a negative register probe avoids detailed footprint construction and pending-event scans. There is no access-plan cache or execution-time recorder. Exceptional permutations, relative registers and FLAT routing use shared pre-execution planning helpers and check control dependencies before reading the control values they need. Keep precise physical-register, lane, byte and completion-order records behind the filter. Track physical VGPRs 0-1023, packed per-wave MSB state, word-precise scalar aliases, partial waits and unordered SMEM completion. Divergence, waterfall reactivation, implicit RMW inputs and matrix lane semantics remain visible to the checker. Eager numerical writeback is not proof of readiness: missing waits produce warnings and execution continues. Remove per-access/TLS checker work, batch wait retirement only when a counter frontier advances, avoid unnecessary overlap rebuilding, reuse classification storage, acquire DS_WRITE2 source spans before its lane loop, and keep scalar reader selection compile-time. These changes also reduce work when checking is disabled. Qualify the generator metadata and exceptional footprints, including scaled MFMA sources and barrier-ID M0 message-result dependencies. Tests explicitly opt in independently of the production default. Matched Release Clang 23 /mnt/dev/smokes measurements use agent-reserved-run, one physical CPU (48), five interleaved repetitions, and XCNT off. The old enabled control is develop 3a6a0d8 plus the configuration-only split in 9aabe33b4328; the implementation is the original accessor-based checker. IREE uses process time; Gluon uses median warm-kernel time. All 80 processes pass numerical checks with zero ordinary/XCNT warnings. Median paired elapsed-time changes (%): workload old on/base new on/base new on/old on new off/base gfx950 iree-f16 +41.46 -1.97 -30.83 -8.40 gfx950 gluon-fp16 +17.42 +4.73 -10.81 -3.39 gfx1100 iree-f32 +11.59 +3.39 -7.50 -1.53 gfx1250 iree-f16 +12.34 +3.36 -7.90 -1.19 The enabled implementation reduces elapsed time by 7.50-30.83% compared with the old enabled checker on these cases. Relative to the new binary with checking disabled, enabling it still costs 7.02% on gfx950 IREE and 8.31% on gfx950 Gluon. Net overhead versus develop includes execution improvements and must not be reported as the isolated cost of a future default flip. These are local workload medians, not universal bounds. Validation: two local peanut-review rounds, Lisa fifth in round two, with all 32 findings resolved; 931 C++ tests passed (84 existing skips), 2402 Python tests passed (46 skips), and all 37 native smoke cells passed. Divergent/waterfall, partial-byte/lane and bank-change regressions included. fMoE is numerically validated but reports independently audited missing VM/LDS waits in the unchanged input object; no workload-specific suppressions were added. The scoreboard checks register readiness, not inter-wave memory visibility or physical GPU timing.
Generate forwarding overrides into typed DS and buffer helpers, sharing wordwise source selection, resource operands, and partial-result masks across 1735 instruction classes. Remove 2395 generated header lines while preserving the instruction hierarchy and central pre-execution checker. Prefer helpers over CRTP bases: the prototype added type metadata and regressed gfx950 IREE and Gluon by 1.03% and 0.84% in five reserved, single-threaded matched pairs. With identical embedded revision metadata, the selected helpers produce byte-identical Release Clang 23 runtime binaries to the current PR. No checking work moves into execution loops. Validation: 4228 assembly/layout comparisons across 2114 instruction classes match; 931 C++ tests pass (84 skips), 2402 Python tests pass (46 skips), and formatting checks pass. Checking stays off by default.
The mixed-FLAT routing tests bypass the issuer, so they no longer create scoreboard entries after dependency registration moved before execution. Decode a real FLAT load, populate its address registers, and register its dependencies before attaching the synthetic routing result. Preserve the per-lane LDS/DDS and global readiness assertions. Brace three conditional EXPECT_EQ calls to avoid GCC dangling-else warnings and bind the selector table entries by const reference. Validation: reproduced both routing failures before the fix; all 17 RoutedMemoryObservationTest cases now pass. GCC 13 syntax-checks both changed test translation units with warnings as errors. The focused 931-test C++ suite also passes. A broader local CTest run has 28 separate translation, DBI, HSA and RCCL fixture/runtime failures; 14 of those tests passed in the original hosted Release job and 14 are absent from it.
Emit SIMPLE_MEMORY_RESULT directly when an instruction has no operand fields requiring a runtime condition. Regenerate all affected targets, removing 203 redundant guards without changing their behavior. Verified that generated changes only remove the constant-true wrappers. Pre-commit passes for the generator and every regenerated file.
When SMEM or XCNT leaves scalar state pending, the generic source check can interpret s_delay_alu and s_sleep immediates as VGPR selectors. Gate VGPR resolution on the existing operand capability so an immediate such as 0x133 cannot alias a pending v51 result. Keep real register reads and the current bank, lane and byte-mask handling intact. Report the actual register intersection when a wide operand overlaps a pending result, rather than naming the operand's first register. Add decoded load/delay/read controls across CDNA4, RDNA3 and CDNA5, including SMEM, XCNT and zero/nonzero EXEC. Both new regressions fail before the fix. Validation: 950 focused tests pass, 84 existing skips; GCC 13 accepts the test and runtime source with warnings as errors; pre-commit passes. Forty reserved single-threaded smoke processes pass with no wait warnings. Median elapsed changes versus the previous enabled checker range from +0.17% to +1.56% across the four workloads.
kuhar
force-pushed
the
users/kuhar/waitcnt-checking-improvements
branch
from
October 8, 2026 01:10
463cdbb to
50dda61
Compare
Apply the effective wave width before resolving explicit vector scalar mask operands. This catches pending VCC_HI destinations in wave32 and avoids false reads of an adjacent SGPR or VCC_HI for condition/carry inputs. Generated metadata distinguishes mask sources from 64-bit data; wave64 masks and f64 scalar pairs still consume both words. Resolve scalar buffer store base words individually, just as scalar buffer loads resolve their consumed descriptor words. Emit the consumed word count in metadata instead of selecting it by architecture at runtime. This preserves the current execution footprints across SGPR/VCC boundaries and backed register prefixes in the ordinary and XCNT consumers. SMEM src_operand(1) now denotes the register offset when present, otherwise the immediate. Require a constant zero offset when recognizing a GOT-slot load, so an s0 selector cannot masquerade as a zero immediate. Route offset rendering through the existing EncodingModifier tables, sharing the architecture-specific no-offset predicate with operand construction. All checking remains before execution, with no per-lane hooks, access-plan cache or execution-time recording. Both diagnostic modes remain opt-in; the implementation and historical overhead comparison are documented in the first commit of this series. Four new regressions fail before these fixes. Validation covers wave32/64, SGPR/VCC mask sources, high-word destinations, true 64-bit data operands, scalar buffer stores and constant/dynamic GOT offsets. The focused suite passes 992 C++ tests (84 existing skips) and 2410 Python tests (46 skips). GCC 13 warning-as-error checks and pre-commit pass. Co-authored-by: Jakub Kuderski <jakub@nod-labs.com>
kuhar
force-pushed
the
users/kuhar/waitcnt-checking-improvements
branch
from
October 8, 2026 01:39
50dda61 to
7513a12
Compare
Contributor
TheRock Submodule Bump ActivityNewest first
|
anujshuk-amd
pushed a commit
that referenced
this pull request
Oct 12, 2026
…13019) Move core-simulator memory-wait checking to once per instruction before execution. Ordinary diagnostics remain off by default, and gfx1250 XCNT remains a separate opt-in, so enabling the defaults can land independently. The existing enabled checker runs through scalar/VGPR accessors during execution, repeating work inside lane and matrix loops. This change checks decoded register identities and widths at the instruction issuer and registers producers before execution or async publication. It replaces atomic shadow loads, thread-local scopes and observer suppression with an issuer-owned 1280-byte register shadow. A negative probe skips detailed footprint construction and pending-event scans. There is no access-plan cache or execution-time recorder. The per-wave scoreboard retains physical VGPR identities 0–1023, packed MSB state, scalar aliases, lane/byte masks, partial waits and unordered SMEM completion. Exceptional permutations, relative registers and FLAT routing use shared planning helpers and check control dependencies before reading the necessary control values. Batched retirement and shared DS source spans also reduce work with checking disabled. Decoded metadata covers immediate-versus-register operands, scalar-memory register offsets, scalar-buffer words crossing SGPR/VCC boundaries and explicit wave-mask inputs/results. Wave32 masks consume one word; true 64-bit data operands retain both words. Multi-register diagnostics identify the pending register. DS/buffer metadata uses shared helpers, and SMEM offsets use the existing modifier tables. GOT-slot discovery now requires a constant zero offset instead of treating the s0 selector as an immediate zero. The following **historical** measurements cover implementation `ecdf4ce06ae`; the subsequent helper-only cleanup preserved identical runtime binaries. Release Clang 23 `/mnt/dev/smokes` runs used `agent-reserved-run`, one physical CPU (48), five interleaved repetitions and XCNT off. The old enabled control is develop `3a6a0d89e39e` plus the configuration-only split in `9aabe33b4328`, retaining the accessor-based checker. IREE uses process time; Gluon uses median warm-kernel time. All 80 processes passed numerical checks with zero wait warnings. Median paired elapsed-time changes: | Workload | Old on / develop off | New on / develop off | New on / old on | New off / develop off | |---|---:|---:|---:|---:| | gfx950 IREE f16 | +41.46% | −1.97% | −30.83% | −8.40% | | gfx950 Gluon f16 | +17.42% | +4.73% | −10.81% | −3.39% | | gfx1100 IREE f32 | +11.59% | +3.39% | −7.50% | −1.53% | | gfx1250 IREE f16 | +12.34% | +3.36% | −7.90% | −1.19% | Net overhead versus develop includes execution improvements. Against the new binary with checking disabled, enabling checking still cost 7.02% on gfx950 IREE and 8.31% on gfx950 Gluon in that experiment. These workload medians do not establish a <=5% bound for a future default flip. The branch is rebased onto develop `33b9dbdc0ee`. It incorporates the reproduced findings from [review 5449610939](#13019 (review)), preserving the contributor's authorship. One subsequent local peanut-review round with Vera, Felix, Petra, Soren and Lisa found four additional edge cases: VCC_HI mask destinations, condition/carry-mask source widths, scalar-buffer-store base dependencies and GOT offsets. All eight comments are addressed in amended head `7513a1292ab`; the fixes were validated locally without a second reviewer round. A final matched comparison of `7513a1292ab` against the initially reviewed/rebased head `50dda613591` used `agent-reserved-run`, one simulator/dispatch thread, five interleaved repetitions and ordinary Warn/XCNT Off. All 40 processes passed with zero warnings. Median paired changes were −0.66% (gfx950 IREE f16), −0.61% (gfx950 Gluon f16 warm), −2.34% (gfx1100 IREE f32), and −0.81% (gfx1250 IREE f16). gfx950 ran on CPU32; the other two targets ran on CPU48. This measures the incremental review fixes, including binary-layout effects, and does not replace the historical develop comparison or measure isolated checking cost. Validation: - 992 focused C++ tests passed, with 84 existing skips; 2,410 Python tests passed, with 46 skips. This includes the FMA-mix cases moved upstream into a separate executable. - Six original review regressions fail before the imported fixes; four additional local-review regressions fail at the reviewed head and pass after the fixes. Controls cover wave32/64, real 64-bit data, completed waits, scalar aliases, divergent/waterfall accesses, partial lanes/bytes, MFMA/WMMA, DPP, permlane/readlane and register banks. DS swizzle has independent numerical coverage. - GCC 13 warning-as-error checks passed for the changed C++ tests and runtime sources; pre-commit passed across all 320 PR files. Regenerated ISA sources are included. The exact amended runtime embeds the commit revision; its final 23 targeted checks passed. - All four hosted corpus configurations passed at `463cdbb5cb1`. The amended head requires fresh hosted CI; local checks are not a substitute for those jobs. The earlier fMoE investigation numerically validated the input while independently confirming missing VM/LDS waits in its unchanged code object. No workload-specific suppressions were added. The scoreboard checks register readiness; it does not establish inter-wave memory visibility or physical GPU timing. --------- Co-authored-by: James Newling <james.newling@gmail.com>
yashjha123
added a commit
that referenced
this pull request
Oct 26, 2026
RDNA scratch addressed each lane's private space contiguously (scratch_base + lane * lane_size + offset). The RDNA3 and RDNA3.5 ISA guides (11.2) define scratch as dword-swizzled with index_stride equal to the wave size: Addr = SCRATCH_BASE + (offset / 4) * 4 * index_stride + offset % 4 + TID * 4 The RDNA4 guide uses the same SWIZZLE(offset, ThreadID) wording, and rocm-dbgapi reads gfx10/11/12 private memory with this 4-byte interleave. CDNA already uses this layout. RDNA's per-lane layout came from #7646, which copied the CDNA convention of the time, and #13019 named it FlatPrivateLayout::Linear to preserve that behavior. Map SCRATCH instructions and private-aperture FLAT lanes through scratch_lane_address(), and mark them as interleaved private lanes so multi-dword accesses split per dword. FLAT wait planning uses the same layout. A spill where every lane uses the same offset now touches one contiguous line instead of one line per lane. FlatPrivateLayout::Linear has no remaining users. Remove it together with FlatAddress::swizzled, which every remaining translation set with private_address; the wait scoreboard now reads private_address. Interleaving moves a lane's high dword a wave stride away, so 8-byte FLAT atomics on private lanes would modify the neighbouring lane's dword. RDNA3/3.5/4 section 11.1 specifies MEMVIOL for them; extend the existing CDNA5 rejection to these targets. Run the layout test on RDNA3 and RDNA3.5 for wave32 and wave64, check mixed private/global FLAT lanes on RDNA3, RDNA3.5 and RDNA4, and update the expectations that encoded RDNA4's per-lane backing. Run the decoded FLAT wait-plan cross-check with b128 loads and returning 32-bit atomics too, so private lanes that split per dword or take the atomic handlers keep the routing execution uses.
yashjha123
added a commit
that referenced
this pull request
Oct 28, 2026
RDNA scratch addressed each lane's private space contiguously (scratch_base + lane * lane_size + offset). The RDNA3 and RDNA3.5 ISA guides (11.2) define scratch as dword-swizzled with index_stride equal to the wave size: Addr = SCRATCH_BASE + (offset / 4) * 4 * index_stride + offset % 4 + TID * 4 The RDNA4 guide uses the same SWIZZLE(offset, ThreadID) wording, and rocm-dbgapi reads gfx10/11/12 private memory with this 4-byte interleave. CDNA already uses this layout. RDNA's per-lane layout came from #7646, which copied the CDNA convention of the time, and #13019 named it FlatPrivateLayout::Linear to preserve that behavior. Map SCRATCH instructions and private-aperture FLAT lanes through scratch_lane_address(), and mark them as interleaved private lanes so multi-dword accesses split per dword. FLAT wait planning uses the same layout. A spill where every lane uses the same offset now touches one contiguous line instead of one line per lane. FlatPrivateLayout::Linear has no remaining users. Remove it together with FlatAddress::swizzled, which every remaining translation set with private_address; the wait scoreboard now reads private_address. Interleaving moves a lane's high dword a wave stride away, so 8-byte FLAT atomics on private lanes would modify the neighbouring lane's dword. RDNA3/3.5/4 section 11.1 specifies MEMVIOL for them; extend the existing CDNA5 rejection to these targets. Run the layout test on RDNA3 and RDNA3.5 for wave32 and wave64, check mixed private/global FLAT lanes on RDNA3, RDNA3.5 and RDNA4, and update the expectations that encoded RDNA4's per-lane backing. Run the decoded FLAT wait-plan cross-check with b128 loads and returning 32-bit atomics too, so private lanes that split per dword or take the atomic handlers keep the routing execution uses.
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.
Move core-simulator memory-wait checking to once per instruction before execution. Ordinary diagnostics remain off by default, and gfx1250 XCNT remains a separate opt-in, so enabling the defaults can land independently.
The existing enabled checker runs through scalar/VGPR accessors during execution, repeating work inside lane and matrix loops. This change checks decoded register identities and widths at the instruction issuer and registers producers before execution or async publication. It replaces atomic shadow loads, thread-local scopes and observer suppression with an issuer-owned 1280-byte register shadow. A negative probe skips detailed footprint construction and pending-event scans. There is no access-plan cache or execution-time recorder.
The per-wave scoreboard retains physical VGPR identities 0–1023, packed MSB state, scalar aliases, lane/byte masks, partial waits and unordered SMEM completion. Exceptional permutations, relative registers and FLAT routing use shared planning helpers and check control dependencies before reading the necessary control values. Batched retirement and shared DS source spans also reduce work with checking disabled.
Decoded metadata covers immediate-versus-register operands, scalar-memory register offsets, scalar-buffer words crossing SGPR/VCC boundaries and explicit wave-mask inputs/results. Wave32 masks consume one word; true 64-bit data operands retain both words. Multi-register diagnostics identify the pending register. DS/buffer metadata uses shared helpers, and SMEM offsets use the existing modifier tables. GOT-slot discovery now requires a constant zero offset instead of treating the s0 selector as an immediate zero.
The following historical measurements cover implementation
ecdf4ce06ae; the subsequent helper-only cleanup preserved identical runtime binaries. Release Clang 23/mnt/dev/smokesruns usedagent-reserved-run, one physical CPU (48), five interleaved repetitions and XCNT off. The old enabled control is develop3a6a0d89e39eplus the configuration-only split in9aabe33b4328, retaining the accessor-based checker. IREE uses process time; Gluon uses median warm-kernel time. All 80 processes passed numerical checks with zero wait warnings. Median paired elapsed-time changes:Net overhead versus develop includes execution improvements. Against the new binary with checking disabled, enabling checking still cost 7.02% on gfx950 IREE and 8.31% on gfx950 Gluon in that experiment. These workload medians do not establish a <=5% bound for a future default flip.
The branch is rebased onto develop
33b9dbdc0ee. It incorporates the reproduced findings from review 5449610939, preserving the contributor's authorship. One subsequent local peanut-review round with Vera, Felix, Petra, Soren and Lisa found four additional edge cases: VCC_HI mask destinations, condition/carry-mask source widths, scalar-buffer-store base dependencies and GOT offsets. All eight comments are addressed in amended head7513a1292ab; the fixes were validated locally without a second reviewer round.A final matched comparison of
7513a1292abagainst the initially reviewed/rebased head50dda613591usedagent-reserved-run, one simulator/dispatch thread, five interleaved repetitions and ordinary Warn/XCNT Off. All 40 processes passed with zero warnings. Median paired changes were −0.66% (gfx950 IREE f16), −0.61% (gfx950 Gluon f16 warm), −2.34% (gfx1100 IREE f32), and −0.81% (gfx1250 IREE f16). gfx950 ran on CPU32; the other two targets ran on CPU48. This measures the incremental review fixes, including binary-layout effects, and does not replace the historical develop comparison or measure isolated checking cost.Validation:
463cdbb5cb1. The amended head requires fresh hosted CI; local checks are not a substitute for those jobs.The earlier fMoE investigation numerically validated the input while independently confirming missing VM/LDS waits in its unchanged code object. No workload-specific suppressions were added. The scoreboard checks register readiness; it does not establish inter-wave memory visibility or physical GPU timing.