fix(rccl): Correct the P2P LL threshold to maintain existing SIMPLE/LL crossover tuning on AMD systems by corey-derochie-amd · Pull Request #12029 · ROCm/rocm-systems

corey-derochie-amd · 2026-09-22T15:41:23Z

Motivation

A reproducible regression was detected in All-to-all 1-node shown here

The regression was bisected to NCCL 2.29.7 sync commit (#6674). Root cause is a change to the SIMPLE/LL crossover calculation.

Technical Details

Adjusted the calculation to maintain the original AMD crossover point.

Issue Tracking

JIRA ID: AICOMRCCL-2141

Test Plan

This is open for discussion. This is an adjustment to restore original calculation and recover performance. A unit test to specifically guard against changes in tuning might be useful, but ultimately brittle. Please advise.

Test Result

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

therock-pr-bot · 2026-09-22T15:42:26Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	⚠️ Warning	Error: Source/code files changed without an accompanying unit test. Expected: add at least one test file named like `test_<name>.py` / `test_<name>.cpp` (or `<name>_test.`). Current:* code file(s) changed: `projects/rccl/src/enqueue/enqueue.cc`, `projects/rccl/test/host/enqueue-test.cc`; no test file found
🚫 Draft PR	🔜 To Be Enabled	—
🚩 Feature Flag	🔜 To Be Enabled	—
📊 Code Coverage	🔜 To Be Enabled	—

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

amd-argus

ARGUS

Bottom line: Worth settling before approval: the widening turns off P2P user-buffer registration for 4 KiB to 64 KiB transfers, so registered or graph-captured alltoall may move the other way.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

amd-argus

ARGUS

Bottom line: Nothing new worth raising; still open from earlier: enqueue.cc:1512 and 1 more. Worth clearing before approval.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

corey-derochie-amd · 2026-10-16T22:15:23Z

@argus re-review

amd-argus · 2026-10-16T22:19:53Z

@corey-derochie-amd the new head (4a88061a) doesn't change any file I'd review. Looks like a merge that didn't touch this PR's own content, so nothing has changed since my last pass above; push a real change and I'll take another look.

alex-breslow-amd

Going to approve, but please add tests.

amd-argus

ARGUS

Bottom line: Nothing new worth raising; still open from earlier: enqueue.cc:1595 and 1 more. Worth clearing before approval.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

amd-argus

ARGUS

Bottom line: Worth settling before approval: the crossover test skips on arch only, so its LL cases fail on PCIe-only gfx950 (MI350P), and LL in the widened band still gives up P2P user-buffer registration.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

corey-derochie-amd · 2026-10-27T21:30:29Z

Going to approve, but please add tests.

@alex-breslow-amd tests added, waiting for CI to complete a test pass.

amd-argus

ARGUS

Bottom line: Worth settling before approval: the new Gfx94 tests pin neither the nNodes/isAllNvlink/LL128 conjuncts nor the p2pChunkSize/8 term, and LL in the widened band still gives up P2P user-buffer registration.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

corey-derochie-amd · 2026-11-05T14:27:17Z

@amd-argus re-review

amd-argus

ARGUS

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

… tuning on AMD systems.

The selection rule is unchanged. State the gfx942/gfx950 cap and the exceptions for a set NCCL_P2P_LL_THRESHOLD and the LL128 path.

…otests Replace the process-isolated SendRecvTests that latched NCCL_P2P_LL_THRESHOLD with focused EnqueueMicrotest cases around addP2pToPlan. This lets the test verify the widened LL cap (up to 64 KiB/peer) and the user-threshold fallback without relying on debug-log parsing or process isolation. Also make the user-threshold check read NCCL_P2P_LL_THRESHOLD on every call, so a later set or unset is visible.

nNodes, isAllNvlink, and the LL128 opt-in each keep 32 KiB on SIMPLE. Dropping any of those checks would select LL, and the new cases fail. Co-authored-by: Cursor <cursoragent@cursor.com>

A 256 KiB chunk stops LL at 32 KiB. A 1 MiB chunk stays capped at 64 KiB. The default 512 KiB chunk makes those two bounds equal. Co-authored-by: Cursor <cursoragent@cursor.com>

ncclLoadParam already ignores NCCL_P2P_LL_THRESHOLD when the value is empty and uses the default. The widen now uses that same rule, and keeps reading it on each call so a later set or unset stays visible. Co-authored-by: Cursor <cursoragent@cursor.com>

NCCL_P2P_LL_THRESHOLD is documented in the user guide, not in env-variables.rst. Describe the unset single-node cap and add the changelog entry for the regression fix. Co-authored-by: Cursor <cursoragent@cursor.com>

corey-derochie-amd · 2026-11-06T16:51:17Z

@amd-argus re-review

amd-argus

ARGUS

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

nNodes > 1 asks rcclParamPxnOptQpUsage, which the host fake aborts on. Disable PXN for that case, matching the production default, so the channel base is computed and the test can check the protocol. Co-authored-by: Cursor <cursoragent@cursor.com>

amd-argus

ARGUS

Bottom line: LGTM.

_{Comment @argus review to re-run, or @argus re-review after pushing.}

corey-derochie-amd requested review from a team September 22, 2026 15:41

github-actions Bot added the project: rccl label Sep 22, 2026

corey-derochie-amd force-pushed the users/corey-derochie-amd/aicomrccl-2141_fix-p2p-ll-threshold branch from 5c26314 to f289fc6 Compare September 22, 2026 16:07

systems-assistant Bot added the organization: ROCm label Sep 22, 2026

amd-argus Bot reviewed Sep 22, 2026

View reviewed changes

Comment thread projects/rccl/src/enqueue/enqueue.cc Resolved

Comment thread projects/rccl/src/enqueue/enqueue.cc Resolved

Comment thread projects/rccl/src/enqueue/enqueue.cc Resolved

corey-derochie-amd force-pushed the users/corey-derochie-amd/aicomrccl-2141_fix-p2p-ll-threshold branch 2 times, most recently from 56c94fd to 3041e03 Compare October 15, 2026 20:17

amd-argus Bot reviewed Oct 15, 2026

View reviewed changes

corey-derochie-amd force-pushed the users/corey-derochie-amd/aicomrccl-2141_fix-p2p-ll-threshold branch from 3041e03 to 4a88061 Compare October 16, 2026 22:15

corey-derochie-amd force-pushed the users/corey-derochie-amd/aicomrccl-2141_fix-p2p-ll-threshold branch from 4a88061 to ab52227 Compare October 22, 2026 19:53

alex-breslow-amd approved these changes Oct 22, 2026

View reviewed changes

alex-breslow-amd reviewed Oct 22, 2026

View reviewed changes

Comment thread projects/rccl/src/enqueue/enqueue.cc Resolved

amd-argus Bot reviewed Oct 26, 2026

View reviewed changes

This comment was marked as off-topic.

Sign in to view

amd-argus Bot reviewed Oct 27, 2026

View reviewed changes

Comment thread projects/rccl/test/SendRecvTests.cpp Resolved

This comment was marked as off-topic.

Sign in to view

corey-derochie-amd changed the title ~~Correct the P2P LL threshold to maintain existing SIMPLE/LL crossover tuning on AMD systems~~ fix(rccl): Correct the P2P LL threshold to maintain existing SIMPLE/LL crossover tuning on AMD systems Oct 27, 2026

corey-derochie-amd requested a review from atulkulk October 27, 2026 21:45

amd-argus Bot reviewed Oct 27, 2026

View reviewed changes

Comment thread projects/rccl/test/host/enqueue-test.cc Resolved

Comment thread projects/rccl/test/host/enqueue-test.cc Resolved

This comment was marked as off-topic.

Sign in to view

atulkulk reviewed Oct 30, 2026

View reviewed changes

Comment thread projects/rccl/src/enqueue/enqueue.cc Resolved

github-actions Bot added the documentation Improvements or additions to documentation label Nov 5, 2026

corey-derochie-amd force-pushed the users/corey-derochie-amd/aicomrccl-2141_fix-p2p-ll-threshold branch from a8f6b36 to 8ec62af Compare November 5, 2026 14:36

amd-argus Bot reviewed Nov 5, 2026

View reviewed changes

This comment was marked as off-topic.

Sign in to view

corey-derochie-amd and others added 9 commits November 6, 2026 10:31

Correct the P2P LL threshold to maintain existing SIMPLE/LL crossover…

3fdc8b0

… tuning on AMD systems.

docs(rccl): shorten the single-node P2P LL threshold comment

b92da40

The selection rule is unchanged. State the gfx942/gfx950 cap and the exceptions for a set NCCL_P2P_LL_THRESHOLD and the LL128 path.

Added tests.

3947d17

test(rccl): lock the gfx94 P2P LL widen gates

2226600

nNodes, isAllNvlink, and the LL128 opt-in each keep 32 KiB on SIMPLE. Dropping any of those checks would select LL, and the new cases fail. Co-authored-by: Cursor <cursoragent@cursor.com>

test(rccl): separate the P2P LL cap from the chunk-size term

4445e45

A 256 KiB chunk stops LL at 32 KiB. A 1 MiB chunk stays capped at 64 KiB. The default 512 KiB chunk makes those two bounds equal. Co-authored-by: Cursor <cursoragent@cursor.com>

docs(rccl): record the gfx94 P2P LL threshold widen

e0c4de8

NCCL_P2P_LL_THRESHOLD is documented in the user guide, not in env-variables.rst. Describe the unset single-node cap and add the changelog entry for the regression fix. Co-authored-by: Cursor <cursoragent@cursor.com>

Fix formatting in enqueue-test.cc

a4479a2

corey-derochie-amd force-pushed the users/corey-derochie-amd/aicomrccl-2141_fix-p2p-ll-threshold branch from 8e863ce to a4479a2 Compare November 6, 2026 16:50

Conversation

corey-derochie-amd commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Issue Tracking

Test Plan

Test Result

Submission Checklist

Uh oh!

therock-pr-bot Bot commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ All Policy Checks Passed

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

corey-derochie-amd commented Oct 16, 2026

Uh oh!

amd-argus Bot commented Oct 16, 2026

Uh oh!

alex-breslow-amd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

corey-derochie-amd commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

Uh oh!

corey-derochie-amd commented Nov 5, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

corey-derochie-amd commented Nov 6, 2026

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as off-topic.

This comment was marked as off-topic.

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

corey-derochie-amd commented Sep 22, 2026 •

edited

Loading

therock-pr-bot Bot commented Sep 22, 2026 •

edited

Loading

corey-derochie-amd commented Oct 27, 2026 •

edited

Loading