perf: eliminate end_sync in custom allreduce by delaying input tensor release until next sync#4084
Open
jpy794 wants to merge 2 commits into
Open
perf: eliminate end_sync in custom allreduce by delaying input tensor release until next sync#4084jpy794 wants to merge 2 commits into
jpy794 wants to merge 2 commits into
Conversation
Co-authored-by: ColorsWind <14761584+ColorsWind@users.noreply.github.com>
Contributor
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in performance mode for the custom collective stack by making the final cross-rank end_sync optional and (when enabled) attempting to preserve correctness by keeping inputs alive across launches from Python.
Changes:
- Thread an
end_syncboolean from Python → pybind → C++ dispatch → HIP kernels for all-gather, reduce-scatter, and fused AR+RMSNorm variants. - Add a
hold_inputsmode toCustomAllreduce(controlled viaAITER_CUSTOM_AR_HOLD_INPUTS) to disableend_syncand retain inputs on the Python side. - Wire the new behavior into
CudaCommunicatorconstruction via an environment variable.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| csrc/kernels/custom_all_reduce.cu | Add end_sync parameter plumbing into C++ entrypoints and dispatchers. |
| csrc/include/rocm_ops.hpp | Expose end_sync as a new pybind argument (defaulting to true) for affected ops. |
| csrc/include/custom_all_reduce.h | Update C++ declarations with end_sync default parameters. |
| csrc/include/custom_all_reduce.cuh | Make kernel-side end_sync conditional for affected kernels/launchers. |
| aiter/ops/custom_all_reduce.py | Update Python op signatures/stubs to include end_sync. |
| aiter/dist/device_communicators/custom_all_reduce.py | Add hold_inputs + input recording, and pass end_sync based on hold_inputs (but currently with correctness gaps). |
| aiter/dist/device_communicators/communicator_cuda.py | Add env-controlled wiring for hold_inputs into CustomAllreduce. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Follow-up to #4082.
PR #4082 fixed a custom collective race by adding an end-of-kernel cross-rank
end_sync, ensuring peer ranks finish reading input buffers before the caller can reuse or overwrite them. That fix is correct, but the extra device-side synchronization adds measurable latency to small custom collective paths.This PR keeps the race fix while providing a performance path that avoids the final
end_sync. Instead of synchronizing at the end of each collective, Python keeps the input tensor alive until the next custom collective launch. The next collective's existing start synchronization proves all ranks have advanced past the previous collective, so the previous input can be safely released at that point.cc @ColorsWind
Technical Details
This change adds an optional
end_syncflag through the custom allreduce Python/C++/HIP stack.When
hold_inputs=False, behavior stays unchanged and kernels still run the finalend_sync.When
hold_inputs=True:end_sync=Falseend_sync<ngpus, true>(...)CustomAllreducerecords the input tensor in PythonThe feature is controlled through:
Covered paths include:
Test Plan
You can find reproduce scripts here https://gist.github.com/jpy794/b21fd3c7b9a9ac1d0e87db62824cf78b
Race repro
Performance microbench
Test Result
Correctness
ptrdenotes input tensor reuse after custom ar.holddenotes value ofAITER_CUSTOM_AR_HOLD_INPUTSPerformance
Submission Checklist