Skip to content

WarpSpeed Refactor and Gating to SPX - #8743

Merged
alex-breslow-amd merged 8 commits into
developfrom
users/abreslow/fixChannelCapFromPR8077
Jul 24, 2026
Merged

alex-breslow-amd merged 8 commits into
developfrom
users/abreslow/fixChannelCapFromPR8077

Conversation

@alex-breslow-amd

@alex-breslow-amd alex-breslow-amd commented Jul 17, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Restore performance on single node for MI350 when WarpSpeed enabled.

Technical Details

New as of 7/22/2026
-Address @Kapil-Shyam-Pawar's feedback regarding changes getting picked up by the testrunner. Thanks for the pointers! :-)

New as of 7/17/2026

  • Synced with team. We disable WarpSpeed on non-SPX modes (e.g., DPX, QPX, CPX). Thanks @mustafabar and @wenkaidu for the suggestion.
  • Move WarpSpeed enablement logic into separate functions that we can unit test
  • Add unit tests for enablement logic
  • Fixes regression in single node performance from PR#8077

Old as of 7/16/2026
Drop in performance due to max channels being capped at 32 for large message sizes on single node. This is one possible fix. Need to verify that it plays nice with CPX and still prevents the issue mentioned in PR#8077.

We need to decide if we should just revert to not using WarpSpeed and go back to the original logic. Will consult with team tomorrow.

Another option is checking which mode we're in e.g., DPX or CPX and having custom max channel counts for that.

Issue Tracking

JIRA ID: AICOMRCCL-1553

Test Plan

Ran with this PR on develop and without it. Verified that performance returns and channel count is again 56 for 1GiB.

Test Result

See above

Submission Checklist

Copilot AI review requested due to automatic review settings July 17, 2026 06:11
@alex-breslow-amd
alex-breslow-amd requested a review from a team as a code owner July 17, 2026 06:11
@therock-pr-bot

therock-pr-bot Bot commented Jul 17, 2026 •

Copy link
Copy Markdown

❌ PR Check — Action Required

Check Status Details
🌿 Branch Name ✅ Pass —
📝 PR Title/Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ❌ Fail Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/rccl/src/enqueue.cc, projects/rccl/src/graph/connect.cc, projects/rccl/src/include/rccl_common.h, projects/rccl/src/init.cc, projects/rccl/src/rccl_wrap.cc (+1 more); no test file found
🔎 pre-commit ⏳ Pending ⏳ Still running…
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

⚠️ 1 policy check(s) failed. Please address the issues above before this PR can be Reviewed.

🚫 Please fix the failed policies

  • ❌ Unit Test

The Not ready to Review label was added to this PR. Once all policies pass, the label is removed automatically.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

@therock-pr-bot

therock-pr-bot Bot commented Jul 17, 2026 •

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ Unit Test

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

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

Restores single-node performance for MI350 when WarpSpeed is enabled by moving the single-node MAXCHANNELS/2 clamp to occur after the gfx950 8-rank single-node halving, avoiding the effective “cap-at-32” behavior introduced by the earlier cap placement.

Changes:

  • Removes the early single-node MAXCHANNELS/2 cap when computing nc under WarpSpeed.
  • Adds a final single-node clamp (nc = min(nc, MAXCHANNELS/2)) after the gfx950 single-node halving logic to preserve higher channel budgets while still protecting comm->channels[MAXCHANNELS] from overflow.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@alex-breslow-amd alex-breslow-amd changed the title Restore channel cap for WarpSpeed after PR#8077 changes WarpSpeed Refactor and Gating to SPX Jul 17, 2026
@wenkaidu

Copy link
Copy Markdown
Contributor

LGTM. I think SPX should be used as gate for other resource intensive algorithm as well, like direct algorithms.

Comment thread projects/rccl/src/init.cc Resolved
Comment thread projects/rccl/src/rccl_wrap.cc Resolved
Comment thread projects/rccl/test/RcclWrapTests.cpp Resolved
@alex-breslow-amd

Copy link
Copy Markdown
Contributor Author

LGTM. I think SPX should be used as gate for other resource intensive algorithm as well, like direct algorithms.

I'll add a follow up task for this. I think that makes sense at least on gfx942 and gfx950.

@alex-breslow-amd

Copy link
Copy Markdown
Contributor Author

Merging in develop because CI failed due to ROCK issue. Fix and bump have landed: ROCm/TheRock@2e7c190 and #9056

@alex-breslow-amd
alex-breslow-amd merged commit 4076c9b into develop Jul 24, 2026
30 of 37 checks passed
@alex-breslow-amd
alex-breslow-amd deleted the users/abreslow/fixChannelCapFromPR8077 branch July 24, 2026 04:21
corey-derochie-amd added a commit that referenced this pull request Jul 28, 2026
Cherry pick commit of #8743 

## Motivation
Restore performance on single node for MI350 when WarpSpeed enabled.

## Technical Details
**New as of 7/22/2026**
-Address @Kapil-Shyam-Pawar's feedback regarding changes getting picked
up by the testrunner. Thanks for the pointers! :-)

**New as of 7/17/2026**
- Synced with team. We disable WarpSpeed on non-SPX modes (e.g., DPX,
QPX, CPX). Thanks @mustafabar and @wenkaidu for the suggestion.
- Move WarpSpeed enablement logic into separate functions that we can
unit test
- Add unit tests for enablement logic
- Fixes regression in single node performance from PR#8077

**Old as of 7/16/2026**
Drop in performance due to max channels being capped at 32 for large
message sizes on single node. This is one possible fix. Need to verify
that it plays nice with CPX and still prevents the issue mentioned in
PR#8077.

We need to decide if we should just revert to not using WarpSpeed and go
back to the original logic. Will consult with team tomorrow.

Another option is checking which mode we're in e.g., DPX or CPX and
having custom max channel counts for that.

## Issue Tracking
JIRA ID: AICOMRCCL-1553

## Test Plan

Ran with this PR on develop and without it. Verified that performance
returns and channel count is again 56 for 1GiB.

## Test Result

See above

## Submission Checklist

- [ ] Look over the contributing guidelines at
https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

(cherry picked from commit 4076c9b)

## Motivation

<!-- Explain the purpose of this PR and the goals it aims to achieve.
-->

## Technical Details

<!-- Explain the changes along with any relevant GitHub links. -->

## Issue Tracking

<!-- Include a GitHub Issue and/or JIRA ID. Do not post any full JIRA
links here. -->
<!-- GitHub issue: #1234 -->
<!-- JIRA ID: EXAMPLECOMPONENT-1234 -->

## Test Plan

<!-- Explain any relevant testing done to verify this PR. -->

## Test Result

<!-- Briefly summarize test outcomes. -->

## Submission Checklist

- [ ] Look over the contributing guidelines at
https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

Co-authored-by: Corey Derochie <161367113+corey-derochie-amd@users.noreply.github.com>
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.

4 participants