Skip to content

Re-enabled MSCCL++ - #1325

Merged
corey-derochie-amd merged 7 commits into
ROCm:developfrom
corey-derochie-amd:finalize-mscclpp
Sep 11, 2024
Merged

Re-enabled MSCCL++#1325
corey-derochie-amd merged 7 commits into
ROCm:developfrom
corey-derochie-amd:finalize-mscclpp

Conversation

@corey-derochie-amd

@corey-derochie-amd corey-derochie-amd commented Sep 6, 2024

Copy link
Copy Markdown
Contributor

Details

Do not mention proprietary info or link to internal work items in this PR.

Work item: Internal

What were the changes?

  • Added restrictions to calling MSCCL++ kernels to match the needs of libmscclpp.
  • Build mscclpp using the cmake flags passed from the rccl cmake process.
  • Link to libmscclpp_nccl statically.
  • Removed IBVerbs dependency.
  • Added better default behaviour and logging.
  • Include mscclpp as a git submodule instead of using download_project.
  • Re-enabled this feature by default.

Why were the changes made?
The MSCCL++ feature had a lot of problems and had been disabled. These fixes address problems to fix unit tests and usability, and re-enable it.

Approval Checklist

Do not approve until these items are satisfied.

  • Verify the CHANGELOG has been updated, if
    • there are any NCCL API version changes,
    • any changes impact library users, and/or
    • any changes impact any other ROCm library.

corey-derochie-amd and others added 7 commits August 29, 2024 18:33
@corey-derochie-amd
Added restrictions around calling MSCCL++ collectives (ROCm#1281)
055d815 
* Added restriction to non-zero 32-byte multiple message sizes to MSCCL++ AllGather.

* Renamed and refactored some mscclpp types.

* Only transmit the MSCCL++ unique id for non-split comm init. For splitting comm, it has already been transmitted. Instead, save the MSCCL++ communicator in child communicators when calling `ncclCommSplit`. Only destroy MSCCL++ communicators when no RCCL communicators remain that use it. Also improved trace logging.

* Disable MSCCL++ when using managed memory buffers as it isn't supported.

* Added datatype and op constraints for MSCCL++ AllReduce.

* Added documentation on MSCCL++ restrictions to the README.
@nileshnegi @corey-derochie-amd
[BUILD] Support custom CMake flags in MSCCLPP (ROCm#1275)
60411c2 
* [BUILD] Support custom CMAKE_PREFIX_PATH in MSCCLPP

Signed-off-by: nileshnegi <Nilesh.Negi@amd.com>

* [BUILD] CMake flags to support build-id in MSCCLPP

Signed-off-by: nileshnegi <Nilesh.Negi@amd.com>

* [BUILD] Fix CMake warnings in MSCCLPP build

Signed-off-by: nileshnegi <Nilesh.Negi@amd.com>

* Wrapped all cmake arguments passed to mscclpp to remove empty arguments and properly format them.

---------

Signed-off-by: nileshnegi <Nilesh.Negi@amd.com>
Co-authored-by: Corey Derochie <corey.derochie@amd.com>
@corey-derochie-amd
Link to libmscclpp_nccl statically (ROCm#1282)
6762d98 
* Switched mscclpp_nccl to static linking. Added a build step to rename the NCCL API functions.

* Undid separation of building libmscclpp_nccl from building librccl with MSCCL++ integration. With a static build, it's either fully enabled or fully disabled.

* `nm` isn't always available in docker containers due to being stripped down. Removed use of `nm` in `cmake` and hard-coded the output into mscclpp_nccl_syms.txt.
@corey-derochie-amd
Removed IBVerbs dependency for integrating with MSCCL++ (ROCm#1313)
7a34891
@corey-derochie-amd
Renamed RCCL_ENABLE_MSCCLPP to RCCL_MSCCLPP_ENABLE to conform to …
41876e0 
…MSCCL. Set `RCCL_MSCCLPP_ENABLE` to 1 by default if `ENABLE_MSCCLPP` is defined, or 0 otherwise. Added a log warning if `RCCL_MSCCLPP_ENABLE` is set to 1 but `ENABLE_MSCCLPP` is not defined. (ROCm#1294)
@corey-derochie-amd
Include mscclpp as a git submodule (ROCm#1314)
f75bfd9 
* Added the desired mscclpp commit as a git submodule.

* Added step to automatically checkout the mscclpp submodule if it isn't already present, in case the user forgot to clone recursively.

* Added instruction to README to clone using --recurse-submodules to get the mscclpp submodule.
@corey-derochie-amd
Enabled MSCCL++ feature build.
87e6054
@corey-derochie-amd corey-derochie-amd self-assigned this Sep 6, 2024
@corey-derochie-amd
corey-derochie-amd merged commit 736a705 into ROCm:develop Sep 11, 2024
@corey-derochie-amd
corey-derochie-amd deleted the finalize-mscclpp branch September 11, 2024 16:13
BertanDogancay pushed a commit that referenced this pull request Mar 28, 2025
@sjeaugey
2.23.4-1
68b5423 
Add scalable init API
 * Add new ncclCommInitRankScalable to allow for passing multiple
   unique IDs to the init function.
 * Spreads the load onto multiple bootstrap roots, allowing for
   constant bootstrap time.
 * Requires multiple ranks to create a unique ID, and the CPU-side
   ID exchange code to call allgather[v] instead of broadcast.

Accelerate init bootstrap operations
 * Reduce the number of calls to allgather.
 * Allow roots to reply early to ranks when information is already
   available.
 * Add an option to use ncclNet instead of sockets to perform
   bootstrap allgather operations.

Add PAT algorithms for Allgather and ReduceScatter
 * Parallel Aggregated Trees, variation of Bruck algorithm.
 * Logarithmic number of network steps for small sizes at scale.
 * Only supports one rank per node at the moment.

Add support for registered buffers for intra-node communication.
 * Allow registered user buffers to be accessed directly intra-node
 * Avoids extra copies in algorithms which permit it, saving
   memory bandwidth and helping with compute overlap.

Add profiler plugin API
 * New plugin API for profiling
 * Supports various levels of profiling, with a hierarchy.

Asynchronous graph allocation
 * Make calls to cudaMalloc and cudaMemcpy during graph allocation
   asynchronous.
 * Significantly speeds up graph capture.

Use fatal IB asynchronous events to stop network operation
 * Avoids many other error messages
 * Only fatal errors are affected; potentially transient errors
   (e.g. port down) do not cause an immediate stop.

Set P2P level to PXB on AMD CPUs when using more than 2 GPUs per node
 * P2P would cause a significant performance degradation when using
   many GPUs, and therefore many interleaved data flows.
 * Disable P2P through the CPU when we have 3+ GPUs per node; keep it
   enabled when we only have 2 GPUs.

Improve the init logs to report the real NCCL function.
 * Make the log report ncclCommInitRank or ncclCommSplit, rather than
   the generic ncclCommInitRankFunc.

Add a parameter to set the location of the user configuration file.
 * Add NCCL_CONF_FILE environment variable to set where the user's
   configuration file resides.

Increase default IB timeout
 * Increase IB timeout value from 18 to 20.
 * Should help avoid fatal errors on large RoCE systems.

Add new check for nvidia peermem
 * On linux kernels 6.6+, /sys/kernel/mm/memory_peers is no longer
   present; check for /sys/module/nvidia_peermem/version instead.

Fix old performance regression when mixing small and large operations.
 * Improves distribution of work on channels.

Fix crash when NUMA IDs are equal to -1.
 * Can happen when a NIC is a virtual NIC, or when linux doesn't
   know which NUMA node a device is attached to
 * Issue NVIDIA/nccl-tests#233

Fix tree graph search when NCCL_CROSS_NIC is set to 1.
 * Would force NCCL to use the balanced_tree pattern, thereby
   disabling LL128 on platforms with 1 GPU+1 NIC per PCI switch.
 * Would also try to use alternate rings even though it was not
   needed.

Compiler tweaks and fixes
 * PR #1177
 * PR #1228

Fix stack smash
 * PR #1325

Fixes for multi-node NVLink + IB operation

Coverity fixes and comments.
corey-derochie-amd pushed a commit to corey-derochie-amd/rccl that referenced this pull request Apr 9, 2025
@sjeaugey
2.23.4-1
e261ece 
Add scalable init API
 * Add new ncclCommInitRankScalable to allow for passing multiple
   unique IDs to the init function.
 * Spreads the load onto multiple bootstrap roots, allowing for
   constant bootstrap time.
 * Requires multiple ranks to create a unique ID, and the CPU-side
   ID exchange code to call allgather[v] instead of broadcast.

Accelerate init bootstrap operations
 * Reduce the number of calls to allgather.
 * Allow roots to reply early to ranks when information is already
   available.
 * Add an option to use ncclNet instead of sockets to perform
   bootstrap allgather operations.

Add PAT algorithms for Allgather and ReduceScatter
 * Parallel Aggregated Trees, variation of Bruck algorithm.
 * Logarithmic number of network steps for small sizes at scale.
 * Only supports one rank per node at the moment.

Add support for registered buffers for intra-node communication.
 * Allow registered user buffers to be accessed directly intra-node
 * Avoids extra copies in algorithms which permit it, saving
   memory bandwidth and helping with compute overlap.

Add profiler plugin API
 * New plugin API for profiling
 * Supports various levels of profiling, with a hierarchy.

Asynchronous graph allocation
 * Make calls to cudaMalloc and cudaMemcpy during graph allocation
   asynchronous.
 * Significantly speeds up graph capture.

Use fatal IB asynchronous events to stop network operation
 * Avoids many other error messages
 * Only fatal errors are affected; potentially transient errors
   (e.g. port down) do not cause an immediate stop.

Set P2P level to PXB on AMD CPUs when using more than 2 GPUs per node
 * P2P would cause a significant performance degradation when using
   many GPUs, and therefore many interleaved data flows.
 * Disable P2P through the CPU when we have 3+ GPUs per node; keep it
   enabled when we only have 2 GPUs.

Improve the init logs to report the real NCCL function.
 * Make the log report ncclCommInitRank or ncclCommSplit, rather than
   the generic ncclCommInitRankFunc.

Add a parameter to set the location of the user configuration file.
 * Add NCCL_CONF_FILE environment variable to set where the user's
   configuration file resides.

Increase default IB timeout
 * Increase IB timeout value from 18 to 20.
 * Should help avoid fatal errors on large RoCE systems.

Add new check for nvidia peermem
 * On linux kernels 6.6+, /sys/kernel/mm/memory_peers is no longer
   present; check for /sys/module/nvidia_peermem/version instead.

Fix old performance regression when mixing small and large operations.
 * Improves distribution of work on channels.

Fix crash when NUMA IDs are equal to -1.
 * Can happen when a NIC is a virtual NIC, or when linux doesn't
   know which NUMA node a device is attached to
 * Issue NVIDIA/nccl-tests#233

Fix tree graph search when NCCL_CROSS_NIC is set to 1.
 * Would force NCCL to use the balanced_tree pattern, thereby
   disabling LL128 on platforms with 1 GPU+1 NIC per PCI switch.
 * Would also try to use alternate rings even though it was not
   needed.

Compiler tweaks and fixes
 * PR ROCm#1177
 * PR ROCm#1228

Fix stack smash
 * PR ROCm#1325

Fixes for multi-node NVLink + IB operation

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

Reviewers

@nileshnegi nileshnegi nileshnegi approved these changes
@wenkaidu wenkaidu Awaiting requested review from wenkaidu
@gilbertlee-amd gilbertlee-amd Awaiting requested review from gilbertlee-amd
@akolliasAMD akolliasAMD Awaiting requested review from akolliasAMD
@edgargabriel edgargabriel Awaiting requested review from edgargabriel
@PedramAlizadeh PedramAlizadeh Awaiting requested review from PedramAlizadeh
@KawtharShafie KawtharShafie Awaiting requested review from KawtharShafie
@AtlantaPepsi AtlantaPepsi Awaiting requested review from AtlantaPepsi
@mberenjk mberenjk Awaiting requested review from mberenjk
@haripriyaayyalasomayajula haripriyaayyalasomayajula Awaiting requested review from haripriyaayyalasomayajula
+1 more reviewer
@nusislam nusislam nusislam approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@corey-derochie-amd corey-derochie-amd

Labels

ci:extended gfx942

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@corey-derochie-amd @nileshnegi @nusislam