revert(rccl): #11562 "ignore RCCL_POISON_HIP_ATOMICS when ENABLE_ROCSHMEM[_GIN]=ON" by omor1 · Pull Request #11727 · ROCm/rocm-systems

omor1 · 2026-09-16T20:37:30Z

Motivation

#11562 is a temporary stopgap to allow RCCL builds with rocSHMEM, i.e. ENABLE_ROCSHMEM=ON, ENABLE_ROCSHMEM_GIN=ON, to succeed until #11350 is merged. Reverting #11562 reenables RCCL_POISON_HIP_ATOMICS now that rocSHMEM has replaced the __hip_atomic builtins with the __scoped_atomic equivalents.

#10785 accidentally reverted support for RCCL_POISON_HIP_ATOMICS entirely; this PR reintroduces that support.

Technical Details

This reverts commit aafcb89.

Issue Tracking

JIRA ID: AICOMRCCL-2340
JIRA ID: AIROCSHMEM-494

Submission Checklist

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

therock-pr-bot · 2026-09-16T20:38:16Z

✅ 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/all_gather.h`, `projects/rccl/src/device/common.h`, `projects/rccl/src/device/reduce_scatter.h`, `projects/rccl/src/include/nccl_device/gin/rocshmem_gda/util.hpp`, `projects/rccl/src/rccl_wrap.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?

alex-breslow-amd

LGTM

amd-argus

ARGUS

Bottom line: Worth settling before approval: whether the pinned rocSHMEM commit at cmake/ROCSHMEM.cmake:27 already carries the __scoped_atomic conversion this revert depends on.

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

Copilot

Copilot review overview

🟡 Changes recommended

Remaining legacy HIP atomic call sites must be migrated or poisoning must remain disabled.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

Global atomic poisoning breaks RCCL device builds · New

What changed in this PR

This PR re-enables RCCL_POISON_HIP_ATOMICS after rocSHMEM’s atomic migration and removes the temporary incompatibility warning.

Changes:

Force-includes the atomic poison header for RCCL builds.
Removes the rocSHMEM incompatibility warning.

File	Summary
`projects/rccl/src/CMakeLists.txt`	Re-enables HIP atomic poisoning; remaining `__hip_atomic_*` call sites cause device builds to fail.
`projects/rccl/CMakeLists.txt`	Removes the obsolete rocSHMEM incompatibility warning.

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

amd-argus

ARGUS

Bottom line: Worth settling before approval: util.hpp still spells _hip_atomic* at lines 112 to 168, so the ENABLE_ROCSHMEM_GIN=ON build this revert re-arms the poison for does not compile.

_{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: CMakeLists.txt:1419 and 1 more. Worth clearing before approval.

_{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: CMakeLists.txt:1486 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 rocSHMEM pin bump reached cmake/ROCSHMEM.cmake:27 but not its twin at install.sh:66, so ./install.sh --rocshmem-gin still checks out the pre-migration rocSHMEM.

_{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: CMakeLists.txt:1479. Worth clearing before approval.

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

…HMEM[_GIN]=ON" This reverts commit aafcb89.

PRs #10785 and #7217 accidentally reintroduced some __hip_atomic builtins. Signed-off-by: Omri Mor <Omri.Mor@amd.com>

f636cc5 (#12380) enables full __scoped_atomic* support in rocSHMEM. Signed-off-by: Omri Mor <Omri.Mor@amd.com>

amd-argus

ARGUS

Bottom line: Nothing new worth raising; still open from earlier: CMakeLists.txt:1516. Worth clearing before approval.

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

systems-assistant · 2026-10-07T16:51:00Z

TheRock Submodule Bump Activity

Newest first

2026-10-07 — Included in TheRock via ROCm/TheRock#8809.

omor1 added this pull request to stack #11728 September 16, 2026 20:37

omor1 requested review from a team as code owners September 16, 2026 20:37

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

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

alex-breslow-amd approved these changes Sep 16, 2026

View reviewed changes

amd-argus Bot reviewed Sep 16, 2026

View reviewed changes

Comment thread projects/rccl/src/CMakeLists.txt Resolved

amirsojoodi requested a review from a team as a code owner September 18, 2026 19:37

omor1 mentioned this pull request Sep 20, 2026

rccl: isolate GIN-SDMA TUs from HIP atomics poison (AICOMRCCL-2221) [1/3] #11893

Closed

3 tasks

omor1 force-pushed the users/omor1/rccl/revert_rocshmem_gin_poison branch from 72d4b63 to 0bee6a6 Compare September 24, 2026 18:23

omor1 requested a lite review from Copilot and removed request for a team September 24, 2026 18:23

Copilot started reviewing on behalf of omor1 September 24, 2026 18:29

View session

Copilot AI reviewed Sep 24, 2026

View reviewed changes

Comment thread projects/rccl/src/CMakeLists.txt Resolved

amd-argus Bot reviewed Sep 24, 2026

View reviewed changes

Comment thread projects/rccl/CMakeLists.txt Resolved

Comment thread projects/rccl/src/CMakeLists.txt Resolved

amd-argus Bot reviewed Sep 24, 2026

View reviewed changes

Base automatically changed from users/asojoodi/rocshmem-scoped-atomics to develop September 25, 2026 13:16

amirsojoodi force-pushed the users/omor1/rccl/revert_rocshmem_gin_poison branch from 723dcfa to 61abe59 Compare September 25, 2026 13:16

omor1 force-pushed the users/omor1/rccl/revert_rocshmem_gin_poison branch from 61abe59 to 16a606c Compare September 28, 2026 21:47

amd-argus Bot reviewed Sep 28, 2026

View reviewed changes

amd-jiali approved these changes Sep 29, 2026

View reviewed changes

omor1 force-pushed the users/omor1/rccl/revert_rocshmem_gin_poison branch from 16a606c to ca85599 Compare September 29, 2026 20:33

amd-argus Bot reviewed Sep 29, 2026

View reviewed changes

Comment thread projects/rccl/cmake/ROCSHMEM.cmake Resolved

omor1 force-pushed the users/omor1/rccl/revert_rocshmem_gin_poison branch from ca85599 to 2356f64 Compare September 29, 2026 21:41

amd-argus Bot reviewed Sep 29, 2026

View reviewed changes

omor1 added 3 commits October 5, 2026 21:09

revert(rccl): #11562 "ignore RCCL_POISON_HIP_ATOMICS when ENABLE_ROCS…

2d622ff

…HMEM[_GIN]=ON" This reverts commit aafcb89.

fix(rccl): replace remaining __hip_atomic builtins with __scoped_atomic

284a3de

PRs #10785 and #7217 accidentally reintroduced some __hip_atomic builtins. Signed-off-by: Omri Mor <Omri.Mor@amd.com>

fix(rccl): pin ROCSHMEM_MONO_HASH to f636cc5 (merge for #12380)

5054b57

f636cc5 (#12380) enables full __scoped_atomic* support in rocSHMEM. Signed-off-by: Omri Mor <Omri.Mor@amd.com>

omor1 force-pushed the users/omor1/rccl/revert_rocshmem_gin_poison branch from 2356f64 to 5054b57 Compare October 5, 2026 22:20

amd-argus Bot reviewed Oct 5, 2026

View reviewed changes

alex-breslow-amd merged commit dbcda78 into develop Oct 6, 2026
84 of 88 checks passed

alex-breslow-amd deleted the users/omor1/rccl/revert_rocshmem_gin_poison branch October 6, 2026 17:55

Conversation

omor1 commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Issue Tracking

Submission Checklist

Uh oh!

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

Uh oh!

✅ All Policy Checks Passed

Uh oh!

alex-breslow-amd left a comment

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

amd-argus Bot 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!

amd-argus Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

systems-assistant Bot commented Oct 7, 2026

TheRock Submodule Bump Activity

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

omor1 commented Sep 16, 2026 •

edited

Loading

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

edited

Loading