Repository navigation
feat(rccl): Scaleup Allreduce using GIN/LSA - #10783
Conversation
❌ PR Check — Action Required
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
Pull request overview
This PR adds GIN/LSA-based “scaleup-only” (single-node) fast paths to RCCL collectives, introducing new SDMA-backed GIN implementations for AllToAll and AllReduce and wiring them into the collective dispatch.
Changes:
- Adds GIN-SDMA AllToAll and AllReduce implementations (new kernels + host entry points) and dispatch hooks in
collectives.cc. - Extends communicator state and teardown to manage GIN internal dev comms and associated resources.
- Updates build/DeviceLinker integration for new HIP objects and adjusts ROCm linking/CI coverage for GIN-SDMA.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/rccl/tools/ci/lib/gin-tests.json | Adds a new CI test variant for internal GIN-SDMA runs. |
| projects/rccl/src/init.cc | Ensures GIN-SDMA resources are finalized during communicator teardown. |
| projects/rccl/src/include/comm.h | Extends ncclComm with GIN-SDMA per-comm state structs and includes new headers. |
| projects/rccl/src/collectives.cc | Routes eligible AllToAll/AllReduce calls to the new GIN-SDMA paths. |
| projects/rccl/src/CMakeLists.txt | Adds new headers/sources and adjusts linking (hsakmt discovery, rocSHMEM host object extraction). |
| projects/rccl/src/algorithms/gin/sdma/gin_alltoall_sdma.cu | New GIN-SDMA AllToAll kernel + eligibility/launch/finalize. |
| projects/rccl/src/algorithms/gin/sdma/gin_all_reduce_sdma.cu | New GIN-SDMA AllReduce algorithms (LSA one/two-shot and GIN two-shot) + state management. |
| projects/rccl/src/algorithms/gin/sdma/all_reduce_gin_sdma.h | New device kernels/utilities backing the AllReduce implementations. |
| projects/rccl/src/algorithms/gin/gin_alltoall.h | Declares AllToAll GIN-SDMA host APIs and per-comm state. |
| projects/rccl/src/algorithms/gin/gin_all_reduce.h | Declares AllReduce GIN-SDMA host APIs, thresholds, and per-comm state. |
| projects/rccl/cmake/DeviceLinker.cmake | Adds device-linker compilation rules for new GIN-SDMA HIP fat objects and adjusts rocSHMEM bitcode behavior. |
Suppressed comments (1)
projects/rccl/src/algorithms/gin/sdma/gin_all_reduce_sdma.cu:116
- msgSize is computed but never used, which can trigger an unused-variable warning (and break builds when warnings are treated as errors).
size_t msgSize = countPerRank * sizeof(T) * comm->nRanks;
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@copilot resolve the merge conflicts in this pull request |
Resolved the merge conflict in |
|
@dlamd1dai , @Kapil-Shyam-Pawar - please review. |
RCCL Perf-Regression Gate:
|
| group | keys | regressions | inconclusive |
|---|---|---|---|
| all_gather_perf-d=bfloat16-default | 0 | 0 | 0 |
| all_gather_perf-d=float-default | 0 | 0 | 0 |
| all_reduce_perf-d=bfloat16-default | 0 | 0 | 0 |
| all_reduce_perf-d=float-default | 0 | 0 | 0 |
| broadcast_perf-d=bfloat16-default | 0 | 0 | 0 |
| broadcast_perf-d=float-default | 0 | 0 | 0 |
| reduce_scatter_perf-d=bfloat16-default | 0 | 0 | 0 |
| reduce_scatter_perf-d=float-default | 0 | 0 | 0 |
|
@dlamd1dai - Please review this PR. |
|
@Kapil-Shyam-Pawar - I have addressed your reviews. Please take a look. |
|
@wenkaidu - Can you please review this PR? |
Co-authored-by: nusislam <25847498+nusislam@users.noreply.github.com>
Co-authored-by: nusislam <25847498+nusislam@users.noreply.github.com>
cdaf1fe to
506b3e4
Compare
There was a problem hiding this comment.
Automated Review Guard for Upfront Scrutiny
Bottom line: No correctness defect found in the kernels. Two things worth settling before merge: the new eligibility suite passes even if ncclAllReduceGinSdmaEligible returns false, and the yield-to-DDA override reroutes symmetric AllReduces GIN never claims.
Comment @argus review to re-run, or @argus re-review after pushing.
dlamd1dai
left a comment
There was a problem hiding this comment.
Re-review of the GIN two-shot default path after the selector and intra-GPU barrier updates. Two follow-ups on ginAllReduceTwoShotKernel.
Co-authored-by: amd-argus[bot] <321830313+amd-argus[bot]@users.noreply.github.com>
Co-authored-by: amd-argus[bot] <321830313+amd-argus[bot]@users.noreply.github.com>
|
The test failures are not related to this PR. |
## Motivation This PR develops scaleup allreduce algorithms using LSA/GIN APIs. ## Technical Details For small/medium message sizes, LSA-based one-shot and two-shot algorithms are used. For large sizes, GIN-based Allreduce kernel is used. Since this is an Allreduce for the scaleup domain, the SDMA-backend for GIN is used for large sizes. This feature requires symmetric memory window registration. This PR is based on ROCm#10386 that has build fixes for GIN SDMA backend. ## Issue Tracking AICOMRCCL-1897 ## Test Plan rccl-tests allreduce unit tests (TODO) ## Test Result For large messages, GIN Allreduce achieves peak bandwidth. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: nusislam <25847498+nusislam@users.noreply.github.com> Co-authored-by: amd-argus[bot] <321830313+amd-argus[bot]@users.noreply.github.com> Signed-off-by: Jihyeon Gim <potatogim@potatogim.net>
TheRock Submodule Bump ActivityNewest first
|
## Motivation This PR develops scaleup allreduce algorithms using LSA/GIN APIs. ## Technical Details For small/medium message sizes, LSA-based one-shot and two-shot algorithms are used. For large sizes, GIN-based Allreduce kernel is used. Since this is an Allreduce for the scaleup domain, the SDMA-backend for GIN is used for large sizes. This feature requires symmetric memory window registration. This PR is based on #10386 that has build fixes for GIN SDMA backend. ## Issue Tracking AICOMRCCL-1897 ## Test Plan rccl-tests allreduce unit tests (TODO) ## Test Result For large messages, GIN Allreduce achieves peak bandwidth. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: nusislam <25847498+nusislam@users.noreply.github.com> Co-authored-by: amd-argus[bot] <321830313+amd-argus[bot]@users.noreply.github.com>
## Motivation This PR develops scaleup allreduce algorithms using LSA/GIN APIs. ## Technical Details For small/medium message sizes, LSA-based one-shot and two-shot algorithms are used. For large sizes, GIN-based Allreduce kernel is used. Since this is an Allreduce for the scaleup domain, the SDMA-backend for GIN is used for large sizes. This feature requires symmetric memory window registration. This PR is based on #10386 that has build fixes for GIN SDMA backend. ## Issue Tracking AICOMRCCL-1897 ## Test Plan rccl-tests allreduce unit tests (TODO) ## Test Result For large messages, GIN Allreduce achieves peak bandwidth. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: nusislam <25847498+nusislam@users.noreply.github.com> Co-authored-by: amd-argus[bot] <321830313+amd-argus[bot]@users.noreply.github.com>
## Motivation This PR develops scaleup allreduce algorithms using LSA/GIN APIs. ## Technical Details For small/medium message sizes, LSA-based one-shot and two-shot algorithms are used. For large sizes, GIN-based Allreduce kernel is used. Since this is an Allreduce for the scaleup domain, the SDMA-backend for GIN is used for large sizes. This feature requires symmetric memory window registration. This PR is based on #10386 that has build fixes for GIN SDMA backend. ## Issue Tracking AICOMRCCL-1897 ## Test Plan rccl-tests allreduce unit tests (TODO) ## Test Result For large messages, GIN Allreduce achieves peak bandwidth. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: nusislam <25847498+nusislam@users.noreply.github.com> Co-authored-by: amd-argus[bot] <321830313+amd-argus[bot]@users.noreply.github.com>
Motivation
This PR develops scaleup allreduce algorithms using LSA/GIN APIs.
Technical Details
For small/medium message sizes, LSA-based one-shot and two-shot algorithms are used. For large sizes, GIN-based Allreduce kernel is used. Since this is an Allreduce for the scaleup domain, the SDMA-backend for GIN is used for large sizes. This feature requires symmetric memory window registration.
This PR is based on #10386 that has build fixes for GIN SDMA backend.
Issue Tracking
AICOMRCCL-1897
Test Plan
rccl-tests allreduce
unit tests (TODO)
Test Result
For large messages, GIN Allreduce achieves peak bandwidth.
Submission Checklist