Skip to content

[ROCDL] Fix wave size for gfx1250 (wave32, not wave64) - #1024

Merged
coderfeli merged 3 commits into
mainfrom
pr/fix-gfx1250-wave-size
Aug 18, 2026
Merged

coderfeli merged 3 commits into
mainfrom
pr/fix-gfx1250-wave-size

Conversation

@sjfeng1999

Copy link
Copy Markdown
Collaborator

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

@sjfeng1999
sjfeng1999 requested review from coderfeli and a lite review from Copilot August 18, 2026 10:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR corrects ROCm target wavefront sizing by introducing a centralized get_warp_size() helper and switching ROCm backend configuration to use it, ensuring gfx1250 is treated as wave32 (not wave64).

Changes:

  • Add get_warp_size() to python/flydsl/runtime/device.py (wave32 for gfx10/gfx11/gfx12*; otherwise wave64).
  • Update ROCm backend target detection and ROCDL wave64 option to use get_warp_size() instead of is_rdna_arch().
  • Update kernel/common helper and docs to reflect the new wave-size logic (but note: the current kernels_common.py change introduces an infinite recursion bug; see PR comments).

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
python/flydsl/runtime/device.py Adds centralized wavefront-size detection used across runtime/backends.
python/flydsl/compiler/backends/rocm.py Uses get_warp_size() for GPUTarget.warp_size and ROCDL wave64 flag selection.
kernels/common/kernels_common.py Attempts to delegate kernel-side get_warp_size() to runtime helper (currently broken due to name shadowing/recursion).
docs/prebuilt_kernels_guide.md Updates documentation for wave-size behavior to include gfx12* (including gfx1250).
CLAUDE.md Updates architecture table notes to reflect corrected gfx1250 wave size behavior.
Suppressed comments (1)

kernels/common/kernels_common.py:102

  • get_warp_size() currently calls itself (return get_warp_size(arch)), leading to infinite recursion. It should delegate to the runtime implementation (aliased at import time).
    """Return the wavefront/warp size for the given GPU architecture.

    CDNA (gfx9xx) uses wave64, gfx10xx/gfx11xx/gfx12xx use wave32.
    """
    return get_warp_size(arch)

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread kernels/common/kernels_common.py Outdated
@sjfeng1999
sjfeng1999 requested review from xudoyuan and a lite review from Copilot August 18, 2026 11:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

python/flydsl/runtime/device.py:106

  • get_warp_size() is now the source of truth for wave size, but the docstring doesn’t call out that wave size is intentionally decoupled from is_rdna_arch() (e.g., gfx1250 is wave32 even though it’s not RDNA). Adding a short note here would help prevent future callers from using is_rdna_arch() for wave-size decisions again.
    """Lanes per warp for an architecture.

    If arch is None, the current GPU arch is auto-detected.
    """

@sjfeng1999
sjfeng1999 force-pushed the pr/fix-gfx1250-wave-size branch from 745b186 to 3951a38 Compare August 18, 2026 11:30
@sjfeng1999
sjfeng1999 force-pushed the pr/fix-gfx1250-wave-size branch from 3951a38 to bd3792d Compare August 18, 2026 11:31
@coderfeli
coderfeli merged commit 3d8fbf1 into main Aug 18, 2026
12 checks passed
@coderfeli
coderfeli deleted the pr/fix-gfx1250-wave-size branch August 18, 2026 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants