Skip to content

[AsyncTP] Set return_A=False in AllGatherGEMMPattern to unlock multim… - #34480

Closed
tianrengao wants to merge 1 commit into
vllm-project:mainfrom
tianrengao:tianren/asynctp-multimem-fix
Closed

tianrengao wants to merge 1 commit into
vllm-project:mainfrom
tianrengao:tianren/asynctp-multimem-fix

Conversation

@tianrengao

@tianrengao tianrengao commented Feb 13, 2026 •

Copy link
Copy Markdown
Contributor

…em path

The AllGatherGEMMPattern replacement calls fused_all_gather_matmul but never uses the gathered tensor (ag_output). However, it relies on the default return_A=True, which prevents the multimem execution path in _should_use_multimem_all_gather_matmul from being selected.

Setting return_A=False signals that the gathered tensor is not needed, enabling the NVLink multicast-based multimem path for small M values. Microbenchmarks on 8xH100 show 1.5x-8.5x per-op speedup for M<=2048.

Note: PyTorch's native Inductor FX pass (micro_pipeline_tp.py) already performs this optimization by checking if ag_output has no downstream users, but vLLM's pattern matcher does not do this liveness analysis.

Purpose

Test Plan

Test Result


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

…em path

The AllGatherGEMMPattern replacement calls fused_all_gather_matmul but
never uses the gathered tensor (ag_output). However, it relies on the
default return_A=True, which prevents the multimem execution path in
_should_use_multimem_all_gather_matmul from being selected.

Setting return_A=False signals that the gathered tensor is not needed,
enabling the NVLink multicast-based multimem path for small M values.
Microbenchmarks on 8xH100 show 1.5x-8.5x per-op speedup for M<=2048.

Note: PyTorch's native Inductor FX pass (micro_pipeline_tp.py) already
performs this optimization by checking if ag_output has no downstream
users, but vLLM's pattern matcher does not do this liveness analysis.

Signed-off-by: tianrengao <terrygao87@gmail.com>
@mergify

mergify Bot commented Feb 13, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @tianrengao.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Feb 13, 2026

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces a performance optimization by setting return_A=False in the fused_all_gather_matmul call within the AllGatherGEMMPattern. As explained in the description, this change is safe because the gathered tensor is not used downstream, and it unlocks a faster NVLink multicast-based execution path for small matrix sizes, resulting in significant speedups. The change is well-contained, correct, and the added code comment clearly explains the rationale. I've confirmed that this optimization is specific to fused_all_gather_matmul and does not apply to fused_all_gather_scaled_matmul used in other patterns, as the latter does not have the return_A parameter. The implementation is solid and I have no further suggestions.

@mergify mergify Bot removed the needs-rebase label Apr 23, 2026
@mergify

mergify Bot commented Apr 23, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @tianrengao.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Apr 23, 2026
@WoosukKwon

Copy link
Copy Markdown
Collaborator

Closing as superseded by #60517, which removed the torch.compile-based sequence parallelism pass and AsyncTP (along with enable_sp,
fuse_gemm_comms, sp_min_token_num, and their tests). The code this PR modifies no longer exists. Thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants