Skip to content

fix(rccl): LL protocol explicit 128-bit vector loads/stores - #10670

Merged
alex-breslow-amd merged 5 commits into
developfrom
users/abreslow/ll_cleanup
Sep 26, 2026
Merged

alex-breslow-amd merged 5 commits into
developfrom
users/abreslow/ll_cleanup

Conversation

@alex-breslow-amd

@alex-breslow-amd alex-breslow-amd commented Aug 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Fixes a potentially serious semantic bug in the codebase that previously required the compiler to fuse two adjacent 8-byte accesses into a single 16-byte access per lane. Instead, explicitly use 16-byte operations for most datatypes. This isn't guaranteed to fix the issue completely though. The compiler could still fragment the 16-byte operation into two or more smaller ones. However, that is less likely.

  • Add loadLLLine / storeLLLine helpers in prims_ll.h, mirroring the LL128 load128NT / store128Plain approach
  • Replace paired 64-bit nontemporal loads and plain stores in LL FIFO access (readLL, readLLBeginAll, readLLFinish, storeLL) with single explicit v4u vector operations
  • Make ncclLLFifoLine always carry a v4u view so the helpers have one layout to work against
  • Preserve the GFX11 inline-asm b128 path, the gfx1250 system-scope path (RCCL_LL_FIFO_SYS_SCOPE, added to develop since this PR was opened), and the gfx1200/gfx1201 store fallback, which now uses __scoped_atomic_store_n to match develop after fix(rccl): replace __hip_atomic builtins with Clang scoped atomics #11321

On the legacy (non-RCCL_LL_FIFO_SYS_SCOPE) paths, a single explicit 128-bit op removes the dependence on the backend fusing two independent 64-bit nontemporal ops into one 16-byte access, and keeps the data and flag words of a line in one memory transaction.

Jira

JIRA ID: ROCM-31751

Test plan

  • Release build on gfx950, gfx1201 and gfx1100 with ROCm 7.0.2 (ONLY_FUNCS="AllReduce RING LL Sum f32|SendRecv"): librccl.so and rcclras link with no new diagnostics. These three targets cover the RCCL_HAVE_GLOBAL_DWORDX4_BUILTINS vector path, the gfx1200/gfx1201 scoped-atomic store fallback, and the __GFX11__ inline-asm arm respectively.
  • gfx1250 build: not covered locally (needs ROCm 7.1.x). The RCCL_LL_FIFO_SYS_SCOPE arm that gfx1250 selects is unchanged by this PR.
  • LL protocol collectives on gfx942/gfx950/gfx1250
  • Verify assembly emits global_load/store_dwordx4 rather than paired flat/dword ops on the HIP paths

Made with Cursor

@alex-breslow-amd
alex-breslow-amd requested a review from a team as a code owner August 24, 2026 22:46
@alex-breslow-amd

Copy link
Copy Markdown
Contributor Author

Stacked on #10666 — base branch is `users/abreslow/ll128_cleanup`, not `develop`.

This PR adds only the LL protocol commit on top of the LL128 changes in #10666:

On the PR header, the base should read into users/abreslow/ll128_cleanup (linked to #10666), not into develop.

@alex-breslow-amd alex-breslow-amd changed the title fix(rccl): use explicit 128-bit vector loads/stores in LL protocol fix(rccl): LL protocol explicit 128-bit vector loads/stores (stacked on #10666) Aug 24, 2026
Comment thread projects/rccl/src/device/prims_ll.h Resolved
@alex-breslow-amd
alex-breslow-amd requested review from a team and a lite review from Copilot August 25, 2026 00:24
@therock-pr-bot

therock-pr-bot Bot commented Aug 25, 2026 •

Copy link
Copy Markdown

✅ 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/device/prims_ll.h, projects/rccl/src/include/device.h; 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?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates RCCL’s LL protocol FIFO access to use explicit 128-bit vector loads/stores, improving correctness by keeping the data and flag words within a single memory transaction (so flag polling can’t observe flags ahead of their associated data).

Changes:

  • Added loadLLLine / storeLLLine helpers in prims_ll.h to perform explicit v4u-based 128-bit loads/stores (including the gfx1200/gfx1201 atomic-store fallback).
  • Replaced paired 64-bit nontemporal loads and paired 64-bit stores in LL FIFO read/write paths with single 128-bit vector operations.
  • Updated ncclLLFifoLine to always carry a v4u view and adjusted header inclusion to support that layout.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
projects/rccl/src/include/device.h Makes ncclLLFifoLine always expose a v4u view and changes how rccl_ptr.h is included.
projects/rccl/src/device/prims_ll.h Introduces loadLLLine/storeLLLine and switches LL FIFO access to explicit 128-bit vector ops.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread projects/rccl/src/include/device.h Resolved
Comment thread projects/rccl/src/include/device.h Resolved

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: Worth settling before approval: the ordering rationale at prims_ll.h:18-20 does not hold for LL, where each 8-byte half carries its own flag and both readers re-check both, so this is a codegen change rather than the correctness fix the title claims.

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

Comment thread projects/rccl/src/device/prims_ll.h Resolved
Comment thread projects/rccl/src/device/prims_ll.h Resolved
Comment thread projects/rccl/src/device/prims_ll.h Resolved
Comment thread projects/rccl/src/device/prims_ll.h Resolved
Comment thread projects/rccl/src/include/device.h Resolved
Comment thread projects/rccl/src/device/prims_ll.h Resolved
alex-breslow-amd and others added 3 commits September 24, 2026 13:36
Replace paired 64-bit nontemporal loads and plain stores in LL FIFO access
with single v4u vector operations, matching the LL128 load128NT/store128Plain
approach. Preserves the gfx1200/gfx1201 dual atomic-store fallback when b128
builtins are unavailable.

Co-authored-by: Cursor <cursoragent@cursor.com>
Apply Praneeth's suggestion

Co-authored-by: Praneeth Vallem <pvallem@amd.com>

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: LGTM.

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

Co-authored-by: Cursor <cursoragent@cursor.com>

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: Nothing new worth raising; still open from earlier: prims_ll.h:18. Worth clearing before approval.

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

@cmcknigh cmcknigh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified on ROCm 10.1 (MI350X / gfx950, Ruby, 2 nodes × 8 GPUs)

This PR fixes the fp32 all_reduce Tree/LL corruption and slowdown seen in the RCCL coco PR smoke on ROCm 10.1. The ROCm 10.2 nightly does not fix it on the compiler side yet, so this change is needed.

Each stack ran two all_reduce_perf -d float -o sum cases with NCCL_IB_QPS_PER_CONNECTION=4:

  • Default: the CI regression case, -b 1K -e 4G -N 5, with the tuner's choice. It picks TREE/LL for 1K–256K.
  • Forced: NCCL_ALGO=TREE NCCL_PROTO=LL, -b 1K -e 1M -N 10.

Rows are size × cycle, and a row is wrong if either the out-of-place or in-place #wrong count is above 0. Times are out-of-place, per call, for TREE/LL at 1K–256K.

ROCm RCCL Default: rows wrong Default: TREE/LL time Forced: rows wrong Forced: TREE/LL time Result
7.14.1 develop c85bb75 0 / 115 39–67 µs 0 / 110 38–58 µs pass
10.0.0 develop c85bb75 0 / 115 39–58 µs 0 / 110 39–58 µs pass
10.1.0rc2 develop c85bb75 10 / 115 1849–2393 µs 17 / 110 1839–2389 µs fail
10.2.0a20260924 (nightly) develop c85bb75 5 / 115 1840–2382 µs 4 / 110 1843–2382 µs fail
10.1.0rc2 this PR 808dc4f 0 / 115 38–57 µs 0 / 110 38–57 µs pass

Root cause (compiler), tracked in ROCM-31751

  • LLVM's SROA pass splits the paired 64-bit __builtin_nontemporal_load reads in LLGenericOp into 32-bit loads, and drops !nontemporal and !tbaa when it does.

  • Each FIFO line's data and flag then end up in separate plain global_load_dwords. That gives torn reads (wrong data) and stale cached polls (the roughly 2.4 ms stall).

  • The SROA behaviour is long-standing. Clang 24's earlier passes are what now hand SROA this IR shape.

  • The 10.2 nightly ships AMD clang 24.0.0git (ROCm/llvm-project@4f43f474), and it still fails the single-function repro:

    opt -mtriple=amdgcn-amd-amdhsa -mcpu=gfx950 -passes=sroa -S sroa_repro.ll
    # in: 10 x load i64 !nontemporal -> out: 4 x load i64 !nontemporal + 12 x load i32 (no !nontemporal)
    

This PR loads each LL line with a single 128-bit v4u nontemporal load, which avoids the split entirely. I'd still land the compiler fix separately, so other code that relies on paired nontemporal loads keeps its atomicity.

…tionale.

Co-authored-by: Cursor <cursoragent@cursor.com>
Comment thread projects/rccl/src/device/prims_ll.h Resolved

@amd-argus amd-argus Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ARGUS

Bottom line: LGTM.

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

@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants