Repository navigation
fix(rccl): update DDA AG threshold - #12678
Merged
Merged
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The focused threshold update matches the stated performance goal and existing boundary tests.
Review effort: Balanced
Findings: None
What changed in this PR
Updates the gfx1250 AllGather protocol crossover so LL128 is used above 64 KiB.
Changes:
- Reduces the AllGather DDA LL ceiling from 128 KiB to 64 KiB.
| File | Description |
|---|---|
projects/rccl/src/rccl_arch_thresholds.cc |
Updates the gfx1250 AllGather LL threshold. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: LGTM on the value itself: DdaFabricEligibilityTests.cpp:476 already asserts 64 KiB and the old entry contradicted it. The only ask is filling in the empty Test Plan.
Comment @argus review to re-run, or @argus re-review after pushing.
isaki001
approved these changes
Oct 2, 2026
Contributor
|
@nawrinsu - Can you test AG on MI350 and see if any adjustment is needed there? |
nawrinsu
changed the title
Contributor
Author
Updated DDA AG threshold from 128MiB to 64MiB based on the perf number shown above in |
alex-breslow-amd
approved these changes
Oct 8, 2026
alex-breslow-amd
left a comment
alex-breslow-amd
left a comment
Contributor
There was a problem hiding this comment.
Approved. Thanks for the optimization.
Contributor
There was a problem hiding this comment.
ARGUS
Findings
1 more finding(s), not posted inline
projects/rccl/test/RcclWrapTests.cpp:3051(Medium) Gfx1250_RankScaledCaps still expects a 128 KiB 4-rank AllGather LL cap; the real table now gives 64 KiB, so update it to 64 * KiB.Gfx1250_RankScaledCaps(RcclWrapTests.cpp:3051) still expects128 * KiBfor the AllGather LL cap on a 4-rank comm built from the real gfx1250 table. BelowrankScaledMinRanks(8),ddaTierCapFromTablereturns the unscaledddaLLMax[AllGather], which this PR sets to 64 KiB. Could we update the expectation to64 * KiB, and the host mirror inMakeRankScaledTableat test/host/wrap-test.cc:1330 so it keeps matching the real table?
Bottom line: LGTM once RcclWrapTests.cpp:3051 expects 64 KiB for the 4-rank gfx1250 AllGather LL cap.
Comment @argus review to re-run, or @argus re-review after pushing.
Contributor
pr-rocm-systems-12678-oci-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
pr-rocm-systems-12678-ruby64-rccl-pr-smokeBuilds
FailuresNo failing tests in this run. 15 tests skipped. |
Contributor
TheRock Submodule Bump ActivityNewest first
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The current max threshold for DDA AG LL protocol on gfx1250 is set to 128KiB. Update this threshold to 64KiB as LL128 outperforms LL at 128KiB.
The current max threshold for DDA AG on gfx950 is set to 128MiB. Update this threshold to 64MiB as Ring outperforms DDA at 128MiB.
Technical Details
Issue Tracking
JIRA ID: AICOMRCCL-2645
Test Plan
Test Result
AG latency(us) on single gfx950 node:
Submission Checklist