Repository navigation
Take the wavefront width from the device, not an assumed 64 - #587
nirvedhmeshram wants to merge 5 commits into
Conversation
gfx1250 is an AMD part with a 32-wide wavefront. Two places assumed AMD
means 64 lanes, and on gfx1250 both Gluon store-path collectives failed to
compile ("Result has an invalid layout ... threadsPerWarp = [64]"):
* Config.threads_per_warp defaulted to 64, and its validator described
32 as NVIDIA and 64 as AMD. It now defaults to the current device's
warp_size; an explicit value still wins.
* gluon/all_to_all.py built its BlockedLayout from literal [64] lanes and
[4] warps. It now reads both from the launch (hip_utils.num_threads and
num_warps), so it also follows config.num_warps.
On gfx1250 at world size 4, tests/ccl/test_all_gather_gluon.py goes from
0/9 to 9/9 and test_all_to_all_gluon.py from 0/9 to 9/9; a new test checks
the default follows the device. The rest of tests/ccl is unchanged.
From #566 (Muhammad Awad), which found and fixed this alongside its TDM
all-gather.
Co-authored-by: Muhammad Awad <muhaawad@amd.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open findings
What changed in this PR
This PR updates Gluon CCL configuration and layouts to derive wavefront/warp width from the active device/launch rather than assuming AMD always uses 64 lanes, fixing compilation on 32-lane AMD parts (e.g., gfx1250).
Changes:
- Default
Config.threads_per_warpnow followstorch’s current devicewarp_sizeunless explicitly provided. - Gluon all-to-all
BlockedLayoutnow derives lanes/warps from Triton launch options instead of hardcoded[64]/[4]. - Documentation/comments and a new test cover the corrected behavior.
| File | Description |
|---|---|
| tests/ccl/test_all_gather_gluon.py | Updates parameter comments and adds a test asserting the default follows the device warp size. |
| iris/ccl/gluon/all_to_all.py | Removes hardcoded layout assumptions; reads warps/lanes from Triton HIP launch metadata. |
| iris/ccl/gluon/all_gather.py | Updates docstring to reflect 32-lane AMD parts. |
| iris/ccl/config.py | Changes threads_per_warp default to auto-detect from the active device; updates validation/error text. |
🧠 Review effort: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
…fault - Config: cache the per-device warp_size query and fall back to 64 when no GPU is available, so Config() stays constructible on CPU-only hosts. - all_to_all (gluon): drop the HIP-only num_threads() helper; take THREADS_PER_WARP from config.threads_per_warp and the warp count from gl.num_warps(), matching the gluon all-gather. - Test the no-GPU fallback. Co-authored-by: Muhammad Awad <muhaawad@amd.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…back - Describe wavefront width by architecture (64 on CDNA3/4, 32 on CDNA5), matching upstream Triton, and drop the NVIDIA mention. - Read the device warp size directly in Config.__post_init__: the query is sub-microsecond, so the lru_cache bought nothing, and the tests already require a GPU, so the no-GPU fallback and its test are removed. Co-authored-by: Muhammad Awad <muhaawad@amd.com> Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
putting this question here since it was under a resolved comment: Why not use |
Go back to hip_utils.num_warps()/num_threads() in the all-to-all layout instead of passing THREADS_PER_WARP from the host. Iris is AMD-only, and these builtins (triton#9604) are in the Triton CI runs. Config keeps its device default because all-gather checks its tile shape against threads_per_warp * num_warps on the host before launch. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@efric re: why not Agreed, switched back in 51e13b4: the all-to-all layout reads I no longer have gfx1250 access, so I checked it by compiling for the target without running it. Built with
|
|
PR description is stale after latest changes. Did you check other launches like gluon unit tests, just to unify change across all cases |
All-gather now reads lanes and warps from hip_utils like all-to-all, and its tile-shape check moves from the host into a gl.static_assert. With no host-side user left, Config.threads_per_warp and its torch device query are removed. The gluon unit tests and examples 06 and 32 hard-coded 64-lane layouts (or a lane count equal to BLOCK_SIZE), which fail to compile on a 32-wide wavefront. They now take the lane count from hip_utils.num_threads(). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@drprajap thanks, both done in 8a912a9.
One thing this turned up, which is outside this PR: Gluon device tracing does not compile for gfx1250. |



gfx1250 (CDNA5) has a 32-wide wavefront (CDNA3/4 are 64). Iris hard-coded 64 lanes in its Gluon layouts, and on gfx1250 both Gluon store-path collectives fail to compile:
Every Gluon layout now reads the wavefront width from the compiler's launch options, via
hip_utils.num_threads()/hip_utils.num_warps()(triton.language.extra.hip.utils, added in triton#9604). Nothing is queried on the host.iris/ccl/gluon/all_to_all.py: theBlockedLayoutused literal[64]lanes and[4]warps. It now useship_utils, so it also followsconfig.num_warpsinstead of assuming 4.iris/ccl/gluon/all_gather.py: tookTHREADS_PER_WARP/WARPS_PER_CTAfrom the host. It now useship_utils. The tile-shape check (block_size_m * block_size_nmust be a multiple of the CTA's thread count) moves from the host launcher into agl.static_assert, with the same message.Config.threads_per_warpis removed, along with its validator. Nothing reads it anymore, and a value that disagrees with the hardware could only fail to compile. API change:Config(threads_per_warp=...)now raisesTypeError.tests/unittests/test_*_gluon.pykernels,test_gluon_cache_modifiers.py,test_device_context_gluon.py, and examples06_message_passingand32_gluon_all_gather_tracingused[64](or[BLOCK_SIZE]) lanes. They now usehip_utils.num_threads().This was found and fixed in #566 (@muhaawad), alongside its TDM all-gather. It's split out here because it's independent of TDM (see #585).
Testing
gfx1250 hardware (first revision of this PR, world size 4). That revision read the width from
Config.threads_per_warprather thanhip_utils:tests/ccl/maintest_all_gather_gluon.pytest_all_to_all_gluon.pytest_all_gather.py,test_all_reduce.py,test_all_to_all.py,test_process_groups.pyCurrent revision, gfx1250, compile only (no hardware access). I compiled the real kernels with
GPUTarget("hip", "gfx1250", 32)and("hip", "gfx950", 64):static_assertfiresstatic_assertfiresload_kernel(unit test), examples 06 and 32The old
[64]literal is a hard compile error against a 32-wide module. CI covers the 64-wide path on MI3xx.Not affected by this PR:
tests/ops/(test_all_gather_matmul,test_matmul_all_gather,test_matmul_all_reduce,test_matmul_reduce_scatter) fail on gfx1250 on unmodifiedmain, with illegal memory accesses and aborts (one hangs). They fail the same way with this change.iris/mem/utils.pyreadsHW_REG_HW_ID/HW_REG_XCC_ID, which the assembler rejects for that target. This affectstest_device_context_gluon.pyafter its layout is fixed.Not included from #566: honouring
config.num_warpsin the two-shot all-reduce launch, and the gfx1250 elements-per-thread guard. No test here exercises them; they can follow separately.🤖 Generated with Claude Code