Repository navigation
feat(kfdtest): Cu mask enablement PR AILIKFD-28 - #9738
Conversation
❌ 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 |
dayatsin-amd
changed the title
|
In the future please use this format for the PR descriptions: |
asierrag-amd
changed the title
| } | ||
|
|
||
| /* | ||
| * Check and record any inactive WPGs. |
There was a problem hiding this comment.
Typo - WPGs - should be WGPs - Workgroup Processors - I know its outside your patch but just noticed.
asierrag-amd
left a comment
asierrag-amd
left a comment
There was a problem hiding this comment.
Hi @shwetagkhatri , just replied to your comments.
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
kfdtest.exclude restructuring, adjustMaskXcc, cuMaskSetUnit refactoring, and ShaderStore gfx12.5 SE-inject all look correct and consistent. No concerns in the kfdtest.exclude blacklist refactor or the mask_config_t extension.
One finding to flag (in addition to what shwetagkhatri raised):
extendedCuMaskingXcc() — In the random subset loop, wgpMask is uint32_t and the loop uses (1u << u) where u can reach 35 on gfx1250 (gridWgpPerXcc=36). 1u << 32 is UB in C++; on x86 GCC's SHL masks the count to 5 bits, so units 32–35 silently re-check bits 0–3. These WGPs are covered by the linear and combination tests, but the random path can never select them. Fix: uint64_t wgpMask and 1ULL << u. (shwetagkhatri caught the truncation side of this independently.)
ShaderStore.cpp — Minor: comment says MSG_RTN_GET_SE_AID_ID[3:0] but the mask is 0x3 which keeps only bits [1:0].
asierrag-amd
left a comment
asierrag-amd
left a comment
There was a problem hiding this comment.
@shwetagkhatri could you please provide your comments
shwetagkhatri
left a comment
shwetagkhatri
left a comment
There was a problem hiding this comment.
I dont want to block approval on KFDTests. Approving along with the following -
Non-blocking nit - the test passes correctly, so this isn't a merge blocker.
But 1u << u at line 1453 is undefined behavior for u = 32..35 (grid is 36). On x86 it happens to wrap onto bits 0–3, so units 32–35 (the 9th WGP of each column) still get toggled, just correlated to units 0–3 rather than independently; unit 31 is effectively never set since rand() is 31-bit.
If you can fix this - the clean form would be:
uint64_t r = ((uint64_t)rand() << 31) | (uint32_t)rand();
uint64_t wgpMask = (r % ((1ULL << gridWgpPerXcc) - 1)) + 1;
for (uint32_t u = 0; u < gridWgpPerXcc; u++)
if (wgpMask & (1ULL << u))
cuMaskSetUnit(mask, cfg, u);
kentrussell
left a comment
kentrussell
left a comment
There was a problem hiding this comment.
Don't really love the exclude format change, but don't have a suggestion to improve it. Especially if we're intending to move the blacklist logic into the binary itself (future plan)
BasicCuMaskingLinear assumes mask bit i selects the i-th CU on the engine that runs the dispatch, which breaks on a multi-XCC node (e.g. gfx12.1.0 in SPX mode) in two ways. The driver spreads the linear mask across XCCs (bit i -> XCC i % NumXcc), but the PM4 queue runs on a single XCC, so most enabled bits never grow the executing XCC's CU count. Stride the mask by NumXcc and cap the sweep at ActiveCU / NumXcc so every enabled CU lands on XCC0. Within that XCC the mask fills round-robin across shader engines, so parallelism grows only in steps of SEsPerXcc. Compare against this SE-balanced effective CU count instead of the raw count. For single-XCC parts (NumXcc == 1) this reduces to the original contiguous mask, so behavior is unchanged.
On gfx12.1 the CU mask ABI is one bit per WGP, interleaved across XCCs (unit u of XCC x is at x + u*numXcc), and a PM4 dispatch runs on a single XCC. The old flat-SE, two-bit-per-WGP flow mis-maps every mask on such parts (e.g. gfx1250), marking all CUs outside XCC0 inactive. Add a per-XCC path (gated on NumXcc > 1 within FAMILY_GFX12): - Let a PM4 queue select its target XCC via BaseQueue::Create; existing callers are unchanged. - Extend mask_config_t with numXcc/targetXcc; numXcc <= 1 keeps the legacy encoding so pre-gfx12.1 parts are unaffected. - Make setCUMask/adjustMask honour the one-bit-per-WGP interleaved encoding and drive/validate each XCC's WGPs in isolation. - Treat KFD topology as WGP-granular (drop the CU<->WGP factor of two) so the per-XCC WGP count and mask size are not halved. Also fix SE readback on gfx12.5+, where HW_ID1 no longer carries SE: the shader reads it from MSG_RTN_GET_SE_AID_ID. Rewrite adjustMaskXcc to mirror KFD's interleaved enumeration and compaction of inactive WGPs, handling asymmetric WGP-per-SA layouts (e.g. SA0=9, SA1=8 on gfx1250). Signed-off-by: Alex Sierra <alex.sierra@amd.com>
Run the CU masking tests on gfx1250 now that they are multi-XCC aware and expected to pass on this ASIC: - Drop KFDQMTest.ExtendedCuMasking from the gfx1250 exclude profile. - Split the CU masking tests (BasicCuMaskingLinear, BasicCuMaskingEven) out of PERMANENT_BLACKLIST_ALL_ASICS into CU_MASKING_BLACKLIST, and add a BLACKLIST_ALL_ASICS_NO_CU_MASK base that omits them. gfx1250 builds on this base so the tests are executed, while all other profiles keep excluding them via BLACKLIST_ALL_ASICS (unchanged behavior).
On gfx12.1 the CU mask ABI is one bit per WGP, interleaved across XCCs (unit u of XCC x is at x + u*numXcc), and a PM4 dispatch runs on a single XCC. The old flat-SE, two-bit-per-WGP flow mis-maps every mask on such parts (e.g. gfx1250), marking all CUs outside XCC0 inactive. Add a per-XCC path (gated on NumXcc > 1 within FAMILY_GFX12): - Let a PM4 queue select its target XCC via BaseQueue::Create; existing callers are unchanged. - Extend mask_config_t with numXcc/targetXcc; numXcc <= 1 keeps the legacy encoding so pre-gfx12.1 parts are unaffected. - Make setCUMask/adjustMask honour the one-bit-per-WGP interleaved encoding and drive/validate each XCC's WGPs in isolation. - Treat KFD topology as WGP-granular (drop the CU<->WGP factor of two) so the per-XCC WGP count and mask size are not halved. Also fix SE readback on gfx12.5+, where HW_ID1 no longer carries SE: the shader reads it from MSG_RTN_GET_SE_AID_ID. Rewrite adjustMaskXcc to mirror KFD's interleaved enumeration and compaction of inactive WGPs, handling asymmetric WGP-per-SA layouts (e.g. SA0=9, SA1=8 on gfx1250). Signed-off-by: Alex Sierra <alex.sierra@amd.com>
613730a to
54c8e26
Compare
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd) — follow-up after gating approvals
Both rocrtst gating co-owners (@shwetagkhatri and @kentrussell) have now approved, which clears the gate. Noting for completeness that the earlier [Optional] item is not yet fixed: in extendedCuMaskingXcc() the random-subset loop is still uint32_t wgpMask = (rand() % ((1ULL << gridWgpPerXcc) - 1)) + 1; with 1u << u, so on gfx1250 (gridWgpPerXcc=36) 1u << u is UB for u>=32 and units 32-35 never appear in random subsets. This is a random-path coverage gap only — the linear and combination sweeps cover those units — so it is non-blocking. A uint64_t wgpMask / 1ULL << u cleanup would close it if you touch this again.
|
Noticed possible infra build flakes, also pulled in develop to get windows fix in ( #10951 ) |
|
Will merge given code owner approval.
All four Build Packages jobs (Linux gfx94X, gfx950, MI455, Windows) are green, and the other required gate Additional notes:
Waiting for this bump #11050 |
Bump the pinned `ROCm/TheRock` ref from `b45a060` (2026-08-26) to `648a501` (2026-09-02) across the TheRock CI workflows. Per Luis Machado's recommendation on #9738: rocm-systems' TheRock ref needs bumping to something recent so those builds pass as they do on TheRock `main`. `648a501` is the current TheRock `main` tip and includes [ROCm/TheRock#7820](ROCm/TheRock#7820) (guard Unix-only `fcntl` import in the Windows driver/GPU sanity check), so this newer ref does not break the Windows test lane. Updates all 15 workflow files that share the develop TheRock pin (21 occurrences: checkout `ref:` lines and rccl `therock_ref` input defaults). The intentionally separate multi-arch pin (`3c4ec01`) is left unchanged. Ref: #9738
TheRock Submodule Bump ActivityNewest first
|
#AILIKFD-28
BasicCuMaskingLinear assumes mask bit i selects the i-th CU on the engine
that runs the dispatch, which breaks on a multi-XCC node (e.g. gfx12.1.0
in SPX mode) in two ways.