Repository navigation
Conversation
New rccl-UnitTestsMicroDdaColl binary for the DDA collective launchers in
src/algorithms/dda/. DdaCollSeams.h renames hipExtLaunchKernelGGL so each
launch is recorded with its typed arguments instead of started, and fakes the
rccl_wrap.cc DDA thresholds the launchers read.
Covers dda_all_gather_{fabric,fabric_ll,fabric_ll128,ipc}.cu and the slot
sizing in all_gather_dda_fabric_ll{,128}.h: eligibility, the scratch check,
grids, unrolled vs runtime kernel choice, and launch arguments.
Co-Authored-By: Claude <noreply@anthropic.com>
Covers dda_alltoall_{fabric,fabric_ll,fabric_ll128,ipc}.cu: eligibility
against scratch and the fixed LL/LL128 slot strides, the staging copy ahead of
the kernel (and IPC's in-kernel copy for single-block payloads), the LL128
grid-shape env overrides and epoch clamp, and launch arguments.
Co-Authored-By: Claude <noreply@anthropic.com>
Covers dda_reduce_scatter_{fabric,fabric_ll,fabric_ll128,ipc}.cu: eligibility,
the float/half/bf16 dispatch, the staging copy, the VMM block budget, the
LL/LL128 grids with their epoch clamp, and launch arguments.
Co-Authored-By: Claude <noreply@anthropic.com>
Covers dda_all_reduce_fabric{,_ll,_ll128}.cu and the slot sizing in
all_reduce_dda_ll{,128}{,_twoshot}.h: the flat/tree choice at 256 KiB, the LL
and LL128 one-shot/two-shot eligibility and tier selection, grids, and launch
arguments. dda_all_reduce_ipc.cu is left to its own tests.
Co-Authored-By: Claude <noreply@anthropic.com>
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
@argus review |
There was a problem hiding this comment.
ARGUS
Findings
4 more finding(s), not posted inline
projects/rccl/test/host/algorithms/dda/alltoall/dda-alltoall-ipc-test.cc:188(Low) Launch_PassesBuffersPerPeerBytesRankAndBarrier never checks that the staged-copy branch forwards stopEventprojects/rccl/test/host/algorithms/dda/DdaCollSeams.h:80(Low) LeakedBarrierState allocates a State per fixture that becomes unreachable, so LeakSanitizer would flag 7 suites under BUILD_ADDRESS_SANITIZERprojects/rccl/test/host/algorithms/dda/all_gather/dda-all-gather-fabric-test.cc:100(Low) Eligibility tests (RanksFromTwoTo144 x11, OnlyFloat32Float16Bfloat16 x14, ZeroCount, NeedsCommAndBootstrap) are copied across the 15 DDA test filesprojects/rccl/test/host/algorithms/dda/reduce_scatter/dda-reduce-scatter-ipc-test.cc:216(Nit) dda-reduce-scatter-ipc-test.cc:216 exceeds the 120-column limit
This PR changes 4,119 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: Looks approval ready.
Comment @argus review to re-run, or @argus re-review after pushing.
Check the IPC alltoall staged-copy launch takes the stop event, keep one reachable barrier state per type so LeakSanitizer stays quiet, and wrap a line over 120 columns. Co-Authored-By: Claude <noreply@anthropic.com>
|
Fixed in 71b776f: the IPC alltoall staged-copy launch is now checked to take the stop event (mutation dropping it is killed); barrier state is one reachable static per type, so LeakSanitizer has nothing to report; the >120-column line is wrapped. The per-file eligibility tests (rank bounds, datatypes, zero count, comm/bootstrap) stay per unit on purpose: each .cu has its own copy of those checks, and a shared helper would hide which unit a failure belongs to. @argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 4,121 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: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
clang >= 23 registers per-kernel profile shadows under -fprofile-instr-generate, which pulled InstrProfilingPlatformROCm into rccl-UnitTestsMicroDdaColl and broke its link in the Multi-Arch build. Weak no-op stubs, as in #13240. Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 4,124 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: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
…comrccl-2826-dda-launchers-host-microtests # Conflicts: # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/test/host/run_host_tests.sh # projects/rccl/tools/scripts/test_runner/configs/ci-precheckin.json # projects/rccl/tools/scripts/test_runner/lib/test_executor.py
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 4,124 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: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
…nBug_ The LL128 threshold seam now returns 0 like rcclDdaLL128Threshold with no arch table and RCCL_DDA_LL128 unset. rccl_assert_source_line pins the DDA params dda-coll-seams.cc copies from rccl_wrap.cc. Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 4,145 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: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
…comrccl-2826-dda-launchers-host-microtests # Conflicts: # projects/rccl/test/host/MICROTEST_README.md
#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>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 4,221 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: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
…comrccl-2826-dda-launchers-host-microtests # Conflicts: # projects/rccl/test/host/MICROTEST_README.md # projects/rccl/tools/scripts/test_runner/lib/test_executor.py
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 4,211 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: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
…comrccl-2826-dda-launchers-host-microtests
#13311 moved the DDA fabric launchers' datatype check into rcclDdaDatatypeSupported() in rccl_wrap.cc and added fp8 dispatch to the reducing launchers. Seam the gate (default mirrors rccl_wrap.cc), check each fabric eligibility defers to it, and pin the fp8 e4m3/e5m2 kernels. Co-Authored-By: Claude <noreply@anthropic.com>
|
@argus review |
There was a problem hiding this comment.
ARGUS
This PR changes 4,283 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: LGTM.
Comment @argus review to re-run, or @argus re-review after pushing.
…ts (#13819) JIRA ID: AICOMRCCL-2826 ## What Fixes the host-test failures on develop: 5 tests in `rccl-UnitTestsMicroDdaColl` and 1 in `rccl-UnitTestsMicroRocmwrap`. ## How #12386 removed the `RCCL_DDA_LL` check from the fabric LL128 eligibility functions (LL128 is gated by `RCCL_DDA_LL128` upstream). #13585, merged a few hours earlier, still expected `RCCL_DDA_LL=0` to reject LL128. The tests now assert `RCCL_DDA_LL=0` leaves LL128 eligible. Develop also reworded the cuMem driver-version warning ("requires ROCm >= 7.12 or the 7.0.2.x backport"); `RocmwrapMicrotest` now looks for that text. ## Test Plan `rccl-UnitTestsMicroDdaColl` 271/271 and `rccl-UnitTestsMicroRocmwrap` 35/35, each 20x shuffled clean (standalone build). Re-adding the old `RCCL_DDA_LL` gate fails the new test. --------- Co-authored-by: Claude <noreply@anthropic.com>
TheRock Submodule Bump ActivityNewest first
|
JIRA ID: AICOMRCCL-2826
What
Host-only microtests for the 15 DDA collective launchers (
src/algorithms/dda/{all_gather,alltoall,reduce_scatter}/dda_*.cu,all_reduce/dda_all_reduce_fabric{,_ll,_ll128}.cu) and their LL/LL128 slot-sizing helpers, in a newrccl-UnitTestsMicroDdaColl.dda_all_reduce_ipc.cuis left to #13240.How
.cuis#included withhipExtLaunchKernelGGLrenamed to a recorder (DdaCollSeams.h), so tests check the kernel, grid, block, stream, stop event and every typed argument; kernels compile as host stubs with registration faked.rccl_assert_source_line. The binary links separately; test(rccl): add host microtests for dda/fabric/fabric_init.cu #13116 defines same-named hooks.Eligible_KnownBug_ScratchCheckUsesPerRankBytesNotGatheredTotal(AllGather fabric and IPC): eligibility checks one rank's bytes against scratch, while the launcher needsnRankstimes that. fix(rccl): fall back from DDA AllGather when the gathered total exceeds scratch #13239 fixes it and flips both pins.Coverage
100% lines on 10 of the 15 launchers and all 6 slot-sizing headers;
dda_all_reduce_fabric_ll.cu96.5% and the four reduce-scatter launchers 97.4-98.1% (unreachableblocks == 0anddefault:arms, and a% 16check the per-rank check implies).Mutation Testing
75 targeted mutations: 68 killed, 7 equivalent (same clamped or rounded value, the implied
% 16check, anll128Thresh == 0test thatbytes > 0covers).Test Plan
Standalone build:
rccl-UnitTestsMicroDdaColl271 tests pass,--gtest_shuffle --gtest_repeat=20clean.