Repository navigation
test(rccl): add Add4G, Recorder, proxy call, LSA and ReduceScatter_LL tests - #12641
Merged
Merged
Conversation
Pin the add4G contract used by the LSA peer pointer math: the delta lands in the high word only, the low word is never touched, the high word wraps mod 2^32, and pointer bases round-trip. Register Add4G.* in the fixtures quick and standard categories and in the precheckin BitOps filter.
Cover the RedOp record() overload in recorder.cc: the PreMulSum branch must serialise scalar, datatype and residence, and RedOpDestroy must serialise op and comm. Register the test in the test_runner configs.
Add flat-VA and IPC-table cases asserting a negative lsaRank returns ncclInvalidArgument instead of resolving below the flat base or reading ipcPeerPtrs[-1].
Drive ncclProxyCallBlocking against a fake proxy on a socketpair and check four paths: a delayed reply is polled until it arrives, a proxy error result ends the poll unchanged, a raised abort flag ends a wait on a silent proxy, and a send failure returns without polling.
Add two inverted pins for current ncclProxyCallBlocking behaviour: an oversized matching reply is received past the caller's respBuff, and an error exit leaves its entry on proxyState->expectedResponses. Each pin carries the post-fix assertions commented out, so the fix PR flips them instead of writing new tests.
Force NCCL_SYM_KERNEL=ReduceScatter_LL, the sole caller of ncclLLA2ASession::send, and check a per-(rank, index) unique float sum so swapped LL data words or misplaced slots fail the oracle. Counts cover the aligned 8-byte and the bounded odd paths. Run on 2 to 8 ranks of one node. Allocate the symmetric windows once and skip collectively when symmetric memory or deviceApiSupport is missing, so the skip names the real reason rather than the ncclInvalidUsage the ineligible forced kernel would return. Only the first count may skip on ncclInvalidUsage, and the per-count checks are MPI-collective so a rank-local failure stops every rank together. Register the test in the mi300x device_api_mpi_lsa config, which already runs 2 co-located ranks with NCCL_CUMEM_ENABLE=1.
Barrier_Lsa_EpochPersistsAcrossLaunches checks that each launch's LSA barrier session starts from the epoch the previous session's destructor saved, so the unicast barrier keeps ordering a cross-rank handoff over repeated launches.
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
Contributor
Author
|
@argus review |
![amd-argus[bot]](https://avatars.githubusercontent.com/in/4740020?s=60&v=4){kind=small}
Fix the BitOps precheckin filter: DIVUP, ROUNDUP and *Hash matched no Suite.Test name, so four suites never ran. Use the Suite.* form. Make the ReduceScatter_LL ncclInvalidUsage skip collective through mpiCoordinatedSkipReason so a diverging verdict cannot hang peers.
Contributor
Author
|
@argus re-review |
Contributor
There was a problem hiding this comment.
ARGUS
Bottom line: Worth settling before approval: ReduceScatterLL_PositionDependentData can skip at four separate points with nothing asserting the verification loop ran, and it is the only SymmetricKernelCorruptionTest registered in any config.
Comment @argus review to re-run, or @argus re-review after pushing.
Synchronize the stream after a successful ncclReduceScatter before the coordinated ineligibility vote, so a skip can never return into ~SymBuf while a kernel still reads the window it deregisters and frees.
Contributor
Author
|
@argus re-review |
Contributor
Author
|
@argus re-review |
Contributor
|
@pvallem the current head ( |
Every case skipped on its own rank's tryAllocSymBuf result, and the short-circuited allocation chain let a failing rank stop registering windows while its peers kept going. Add allocSymBufsSkipReason, which registers every buffer regardless and then votes through mpiCoordinatedSkipReason, and use it in all seven cases.
Contributor
Author
|
@argus re-review |
sdarisala
approved these changes
Oct 2, 2026
Only the JSON path of Recorder RedOp records is tested, so raw rcclApiCall fields written in binary mode can regress unseen. Fix: read both RedOp records back from the binary dump and check each field; stacked on #12641. JIRA ID: AICOMRCCL-2699
Contributor
Author
|
@argus re-review |
BertanDogancay
approved these changes
Oct 5, 2026
Contributor
TheRock Submodule Bump ActivityNewest first
|
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.
add4G, Recorder RedOp records, negative lsaRank, ncclProxyCallBlocking error paths, ReduceScatter_LL placement and the LSA barrier epoch had no tests.
Fix: add a test for each, plus two pins of known proxy response defects; the symmetric-kernel tests skip without cuMem.
JIRA ID: AICOMRCCL-2694, AICOMRCCL-2697, AICOMRCCL-2698, AICOMRCCL-2699, AICOMRCCL-2700, AICOMRCCL-2704