Repository navigation
feat(rccl): Add LL128 support for DDA RS - #12928
Conversation
|
No undocumented RCCL or NCCL environment-variable changes. |
❌ 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.
Copilot review overview
🟡 Changes recommended
The LL128 tier is not selected under the default configuration, and IPC medium messages can be copied twice.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Adds LL128 protocol support for DDA fabric reduce-scatter and optimizes IPC staging.
Changes:
- Implements LL128 reduce-scatter packing, synchronization, and reduction.
- Derives launch and scratch geometry dynamically.
- Adds eligibility tests and medium-message IPC staging.
| File | Description |
|---|---|
DdaFabricEligibilityTests.cpp |
Adds LL128 eligibility boundary tests. |
reduce_scatter_dda.h |
Adds in-kernel IPC staging. |
reduce_scatter_dda_fabric_ll128.h |
Implements the LL128 device kernel. |
dda_reduce_scatter_ipc.cu |
Selects IPC staging strategy. |
dda_reduce_scatter_fabric_ll128.cu |
Adds LL128 launch geometry and eligibility. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@cppccppccppc @sunguobao PTAL |
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: ReduceScatterLL128_UnalignedBytes passes 4 bytes, which the old % 8 gate rejected too, so nothing in the suite pins the new 16-byte rule.
Comment @argus review to re-run, or @argus re-review after pushing.
nusislam
changed the title
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: the ReduceScatter LL128 cap going 2 MiB to 128 MiB makes the rewritten ddaReduceScatterFabricLL128 the default path over a range it has never served, with no opt-in.
Comment @argus review to re-run, or @argus re-review after pushing.
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: the gfx1250 default still routes reduce-scatter in (1 MiB, 64 MiB] onto the rewritten LL128 kernel, and the new MPI cases are registered only on gfx942 and gfx950 lanes where their gfx1250 gate always skips.
Comment @argus review to re-run, or @argus re-review after pushing.
97c0013 to
a3d07a4
Compare
|
The code coverage CI failures are not related to this PR. There are failures for RMA, CE, DDA Alltoall and Allgather tests. |
|
@nawrinsu, @srirajpaul - Please review. |
|
@copilot resolve the merge conflicts in this pull request |
…-rs-ll128 # Conflicts: # projects/rccl/test/DdaReduceScatterMPITests.cpp Co-authored-by: nusislam <25847498+nusislam@users.noreply.github.com>
Merged current |
There was a problem hiding this comment.
ARGUS
This PR changes 1,011 lines across 22 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: Worth settling before approval: removing kDdaLL128RsMaxBytes leaves DdaReduceScatterMPITests.cpp:139 unable to compile, and that suite's odd-8B-word LL128 shapes now fail the new % 16 gate.
Comment @argus review to re-run, or @argus re-review after pushing.
There was a problem hiding this comment.
ARGUS
This PR changes 1,524 lines across 23 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: Worth settling before approval: the updated DdaReduceScatterMPITests.cpp landed at the monorepo root, so the built copy in projects/rccl/test still uses the removed kDdaLL128RsMaxBytes and rccl-UnitTestsMPI will not compile.
Comment @argus review to re-run, or @argus re-review after pushing.
0b6a7f1 to
33ab2e9
Compare
There was a problem hiding this comment.
ARGUS
This PR changes 1,090 lines across 23 files. A change this size is hard to review
well, for a person and for this bot alike: reviewers lose the thread, defects hide in the
volume, and the feedback arrives too late to be cheap to act on. Splitting it into a
stacked pull request
gives each piece a reviewable diff and keeps the history. This review covers the whole change
regardless.
Bottom line: Nothing new worth raising; still open from earlier: rccl_arch_thresholds.cc:47 and 2 more. Worth clearing before approval.
Comment @argus review to re-run, or @argus re-review after pushing.
#12928 moved the fabric LL128 reduce-scatter to the warp-per-slice layout (512 threads, 16-byte shards and buffers, the LL128 cap, a slot sized from the scratch bank) and dropped DDA_LL128_RS_THREADS; the IPC launcher now copies shards up to 1 MiB in the kernel and takes the stop event before the host staging copy. Co-Authored-By: Claude <noreply@anthropic.com>
TheRock Submodule Bump ActivityNewest first
|
## Motivation The coco PR gate built its develop baseline at develop tip on every run, because each run uses a fresh output directory. A PR was therefore measured against code it did not contain. In 18 replayed ruby64 runs, seven unrelated PRs showed another commit's 1-node reduce_scatter change (#12928) as their own. ## Technical Details `rccl_resolve_coco_run.py` now, for PR runs: - resolves the PR's base branch (`GET /pulls/{n}`) and its merge-base with the head (`GET /compare/{base}...{head}`), using the job's `GITHUB_TOKEN`; - appends `--baseline-sha <merge-base>` to `coco_args`, which already flows into both the build-only and the test `coco pr` calls; - fails the setup job when the merge-base cannot be resolved or equals the PR head, rather than silently comparing the wrong code. API failures name the request and status. Only server errors, rate limits and transport failures (timeouts, resets, truncated responses) are retried. **When the pin applies.** It is passed only when the checked-out coco harness declares `--baseline-sha` (the probe searches `coco/cli/`). A harness without the flag gets a workflow warning and the baseline stays on its configured ref. The new `require_baseline_sha` action input (default `false`) turns that warning into a setup failure. **The PR gate requires the pin.** #14389 moved the default harness to `1.4.x`, which declares `--baseline-sha`, so the second commit passes `require_baseline_sha: 'true'` in `rccl-coco-pr.yml`. A harness that loses the flag now fails setup instead of silently measuring against develop tip. The scheduled workflow never runs in PR mode, so it is unchanged. Rolling `coco_ref` back to `1.3.x` has to turn the input off as well, and a comment in the workflow says so. The wiring test reads the action's `action.yml`, which the RCCL script-test job (`rccl-cluster-runner-allowlist.yml`) did not check out. That workflow's sparse checkout now includes `.github/actions/rccl-resolve-coco-run`, and its `paths` filter includes that directory, so an edit to the action's inputs re-runs the test that guards them. The action also passes the harness location explicitly (`HARNESS_DIR`) instead of relying on the step's working directory. Periodic runs are unchanged. ## Issue Tracking JIRA ID: AICOMRCCL-2204 ## Test Plan - `.github/scripts/tests/rccl_resolve_coco_run_test.py`: 50 tests pass when run directly, with or without an HTTP proxy in the environment. `python3 -m pytest .github/scripts/tests/*rccl*_test.py` gives 157 passed, 2 skipped. They cover: - args with and without a pin; - merge-base resolution against the PR's own base branch, a base branch with reserved characters, an uppercase head, a head that is its own merge-base, and a malformed or incomplete response; - the harness probe on a moved, single-quoted or multi-line declaration, and on a harness that only has `--baseline-stack`; - GitHub failures: a 404 and a malformed URL fail at once with the request named; 5xx, rate limits, timeouts, connection resets and truncated bodies are retried; a missing token; a non-JSON body; - `main()` end to end: missing `HARNESS_DIR`, a periodic run, a required pin on an old harness, and an API failure; - one test against a real loopback socket that resets the connection after the request, through a direct (no-proxy) opener; - the wiring: the action declares `require_baseline_sha` (default off) and the PR workflow passes it, since GitHub only warns about an undeclared input. - Formatted with the repo's black 26.5.1. - Mutation-checked: reverting retry-on-404, the literal-only probe, the lowercasing, the URL quoting, the transient-error clause, the `InvalidURL` clause and a misspelt workflow input each makes at least one test fail. - Against the real API: this PR's merge-base resolves to `c7047ffc473d` (matching `git merge-base`), an uppercase head resolves the same, and a nonexistent PR fails in 0.2 s with the request in the message. ## Test Result All pass. This PR's own gate run is the first end-to-end check: with the `1.4.x` default harness, the `coco pr` command lines should show `--baseline-sha` and the baseline stack's RCCL version line should be the PR's merge-base. Without this change the gate runs the `1.4.x` harness but still builds its baseline at develop tip. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md. --------- Signed-off-by: speriaswamy-amd <surya.periaswamy@amd.com>
Motivation
This PR adds LL128 protocol support for DDA RS in the fabric path.
Technical Details
This PR adds LL128 protocol support for DDA RS in the fabric path. It also moves the copies to temporary buffer inside the kernel for some message sizes in the medium range
[internal-env].Issue Tracking
AICOMRCCL-2471
Test Plan
rccl-test, unit tests
Test Result
Submission Checklist