Skip to content

[HIP] [Bugfix] Skip empty inputs in custom all-gather - #7302

Merged
TennyWang1223 merged 1 commit into
ROCm:mainfrom
XinyuJiangCMU:pr/empty-custom-allgather-20261021
Nov 4, 2026
Merged

TennyWang1223 merged 1 commit into
ROCm:mainfrom
XinyuJiangCMU:pr/empty-custom-allgather-20261021

Conversation

@XinyuJiangCMU

@XinyuJiangCMU XinyuJiangCMU commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Skip empty inputs in the custom all-gather: return from dispatchAllGather before the buffer lookup, and skip the zero-byte hipMemcpyAsync in all_gather_unreg.

Motivation

With attention data parallelism (DP), a DP group that has no requests in a step still joins the collectives, with empty inputs. RL rollouts do this routinely: groups that finish early wait for the longest requests. When a DP group has more than one GPU (attention TP > 1), its all-gather then reaches AITER with an empty input, and dispatchAllGather launches its kernel with zero blocks. The next kernel that checks for errors fails with HIP runtime error: invalid configuration argument. should_custom_ag accepts size zero while the all-reduce and reduce-scatter checks reject it, so all-gather is the only path that gets here.

Changes

  • dispatchAllGather returns before the buffer lookup when the input is empty, so no zero-block kernel is launched.
  • all_gather_unreg skips the zero-byte hipMemcpyAsync, whose source pointer is null for an empty tensor.
  • Non-empty inputs take the same path as before.

Testing

  • Unit tests added or updated
  • Performance benchmarks run
  • Tested on MI250X
  • Tested on MI300X

Tested on MI355X (gfx950); see "Validation and unverified scope" below.

Documentation

  • Docstrings updated
  • User guide updated
  • Performance guide updated
  • No documentation change is needed, with the reason stated below

This is an internal guard for empty inputs; no API changes.

Dependencies

  • No new third-party dependencies added
  • New dependencies are listed and justified below

Breaking Changes

None.

PR Self-Review

Reviewed revision

5a40117

Behavior and intent

An empty all-gather input now returns immediately instead of launching a kernel with zero blocks. Before, the next kernel that checked for errors failed with HIP runtime error: invalid configuration argument, which stopped RL rollouts with attention DP whenever a DP group was idle. Non-empty inputs are unchanged.

Validation and unverified scope

On MI355X with AITER e7d2453 plus this change:

  • 72 two-rank API checks pass across FP32, FP16 and BF16, empty and non-empty inputs, registered and unregistered calls, first- and last-dimension gather, and graph replay on the copy-in (all_gather_unreg) path.
  • A 4-GPU Qwen3.5 BF16 RL job with attention DP and MoRI that failed with this error completes two rollouts and four training steps.
  • On main, the patched functions and the all-gather path they guard are identical to the tested build, and the patch applies to current main.

Unverified: graph replay on the registered (all_gather_reg) path, which was tested in eager mode only, and runs on MI250X and MI300X.

PR-specific question

The main boundary is that every rank has to skip together. A standard all-gather requires the same input size on every rank, so an empty collective is empty on all ranks of the group and they all return early; no rank waits at a barrier the others skipped. For non-empty inputs the only change is one extra size check.

Changed op tests

None.

Co-author-with: @JessicaJiang-123

Return from dispatchAllGather before the buffer lookup when the input is
empty, and skip the zero-byte hipMemcpyAsync in all_gather_unreg, whose
source pointer is null for an empty tensor.

Signed-off-by: Xinyu Jiang <xinyuj2@andrew.cmu.edu>
Co-authored-by: Zhiyao Jiang <jessicajiang324@gmail.com>
@XinyuJiangCMU
XinyuJiangCMU requested review from a team and a balanced review from Copilot November 2, 2026 04:55

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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@github-actions github-actions Bot changed the title [Bugfix] Skip empty inputs in custom all-gather [HIP] [Bugfix] Skip empty inputs in custom all-gather Nov 2, 2026
@github-actions github-actions Bot added the HIP label Nov 2, 2026
@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every PR:

  • ✅ Pre-checks (submodule verification, code formatting)
  • ✅ Aiter op tests (gfx942 + gfx950)
  • ✅ Triton tests on MI35X (only when aiter/ops/triton/** or related paths are changed)

Extended tests (opt-in via labels):

Label Tests
ci:gfx1250-ffm-triton Run the five-shard gfx1250 FFM Triton test suite
ci:triton-300x Run an additional Triton test job on MI300X in PRs (added automatically when gfx942 configs change); main branch always runs both MI35X and MI300X
ci:triton-355 Run the full Triton test suite on MI35X, not only the tests the change affects
ci:windows Windows JIT op smoke test on the gfx1201 ROCm runner
ci:multi-gpu Aiter multi-GPU tests on the 8-GPU runner
ci:sglang SGLang integration tests: DeepSeek-R1-MXFP4 accuracy, Qwen 3.5 accuracy
ci:atom ATOM benchmark: DeepSeek-R1-0528, GPT-OSS-120B
ci:atom_full ATOM accuracy suite for PR and main models from ATOM models_accuracy.json
ci:vllm vLLM benchmark: DeepSeek-V4-Pro, MiniMax-M3-MXFP4
ci:all All standard extended tests (excludes ci:atom_full)

Only add ci:atom_full for FlyDSL or Triton upgrades.
Add labels via the sidebar or gh pr edit 7302 --add-label <label>

One backend per PR:
A PR changes one kernel backend: [Triton/Gluon] (Triton and Gluon count as one), [HIP], [ASM], [CK], [OPUS] or [FlyDSL]. If the title ends up with two backend tags, split the PR -- as stacked pull requests when one part cannot merge without the other.

PR title tags & labels:
Component tags ([Triton/Gluon], [HIP], [CK], [ASM], ...) are added to the PR title and as PR labels automatically from the changed files and re-synced on every push — change-type tags like [fix]/[Perf], op tags like [MLA], and human labels (ci:*) are left untouched. Add the no-auto-title label to stop the title rewrites; labels stay in sync either way.

@aiter-bot

aiter-bot commented Nov 2, 2026 •

Copy link
Copy Markdown

READY FOR HUMAN REVIEW

PR Self-Review

Revision 5a40117a3741 · mode shadow

Author self-review accepted. Readiness report:

Author answers

  • PASS — current-SHA answers are present and structurally specific
Show the author's self-review answers

Behavior and intent

An empty all-gather input now returns immediately instead of launching a kernel with zero blocks. Before, the next kernel that checked for errors failed with `HIP runtime error: invalid configuration argument`, which stopped RL rollouts with attention DP whenever a DP group was idle. Non-empty inputs are unchanged.

Validation and unverified scope

On MI355X with AITER e7d2453f plus this change:

- 72 two-rank API checks pass across FP32, FP16 and BF16, empty and non-empty inputs, registered and unregistered calls, first- and last-dimension gather, and graph replay on the copy-in (`all_gather_unreg`) path.
- A 4-GPU Qwen3.5 BF16 RL job with attention DP and MoRI that failed with this error completes two rollouts and four training steps.
- On main, the patched functions and the all-gather path they guard are identical to the tested build, and the patch applies to current main.

Unverified: graph replay on the registered (`all_gather_reg`) path, which was tested in eager mode only, and runs on MI250X and MI300X.

PR-specific question

The main boundary is that every rank has to skip together. A standard all-gather requires the same input size on every rank, so an empty collective is empty on all ranks of the group and they all return early; no rank waits at a barrier the others skipped. For non-empty inputs the only change is one extra size check.

Changed op tests

None.

Co-author-with: @JessicaJiang-123

PR state

  • PASS — pull request is marked ready for review

Copilot findings

  • PASS — no active Copilot finding lacks a substantive author reply
  • ADVISORY — Copilot could not review the current head (for example, quota or service failure)

Code and op-test hygiene

  • PASS — no committed artifact or conflict marker detected

Required CI

  • PASS — Black: passed
  • PASS — Ruff: passed
  • PASS — Repository dependency: passed
  • PASS — Aiter Test Gate: passed

Shadow mode reports what would block but keeps the status successful. Set AITER_PR_SELF_REVIEW_MODE=enforce only after reviewing real false positives.

@TennyWang1223 TennyWang1223 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.

LGTM

@TennyWang1223
TennyWang1223 merged commit e4973c4 into ROCm:main Nov 4, 2026
55 checks passed
yuychang pushed a commit to yuychang/aiter that referenced this pull request Nov 4, 2026
…usal

- Pick up ROCm#7088's merge with main (ROCm#6983 Q-axis configs through ROCm#7302) so this stacked PR is conflict-free again.
- Pick up ROCm#7088's SPLIT_BLOCK_M, which keeps q4 draft verify blocks at 16 rows under main's BLOCK_M 32 entry.
- No textual conflicts; the CAUSAL constexpr composes with the merged split code.
yuychang added a commit to yuychang/aiter that referenced this pull request Nov 4, 2026
…usal

- Pick up ROCm#7088's merge with main (ROCm#6983 Q-axis configs through ROCm#7302) so this stacked PR is conflict-free again.
- Pick up ROCm#7088's SPLIT_BLOCK_M, which keeps q4 draft verify blocks at 16 rows under main's BLOCK_M 32 entry.
- No textual conflicts; the CAUSAL constexpr composes with the merged split code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants