Skip to content

transport/common: use the CMake-defined x86_64 macro for host barriers - #125

Open
0z5a wants to merge 1 commit into
NVIDIA:develfrom
0z5a:fix-x86-barrier-macro
Open

0z5a wants to merge 1 commit into
NVIDIA:develfrom
0z5a:fix-x86-barrier-macro

Conversation

@0z5a

@0z5a 0z5a commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Incremental review: one line in src/modules/transport/common/transport_common.h, against devel (2dca770).

Fixes #124.

Problem

transport_common.h selects its x86 MEM_BARRIER() / STORE_BARRIER() / LOAD_BARRIER() with #if defined(NVSHMEM_x86_64). CMake defines only NVSHMEM_X86_64 (src/CMakeLists.txt:533, src/modules/transport/CMakeLists.txt.in:88, src/device/CMakeLists.txt:131). On x86_64 the macros therefore fall back to the generic compiler-only barrier, and none of the STORE_BARRIER() calls in ibdevx, ibrc, gpunetio, libfabric and staged emits sfence. That includes staged_gdrcopy.cpp:194, whose comment says it is there to drain the write-combining buffers.

Fix

Test NVSHMEM_X86_64, the name every other x86 check in the transports already uses.

Validation

  • git grep NVSHMEM_x86_64 returns nothing after the change.
  • I cross-compiled the barrier block, unchanged, in front of a function shaped like nvshmemt_ibdevx_post_send (clang 21, -O2, with the define CMake passes):
    • x86_64 before: no fence instructions.
    • x86_64 after: sfence before the doorbell-record store and before the BlueFlame store.
    • aarch64 (-DNVSHMEM_AARCH64): dmb st in both cases.
  • What the change adds on x86_64: sfence at the 8 STORE_BARRIER() call sites, which are the only ones in the tree. MEM_BARRIER() and LOAD_BARRIER() have no callers, so no mfence or lfence appears. The two sites in CQ progress loops (ibdevx.cpp:1181, gpunetio.cpp:697) run once per consumed CQE.
  • I built devel plus this change on aarch64 (GH200) with -DCMAKE_CUDA_ARCHITECTURES=103 and tests enabled. That build does not compile the x86 branch, so it only shows that nothing else broke.
  • I did not build the patched tree on an x86_64 host, and I did not run an x86 IB latency A/B. No performance claim.

The commit carries a DCO sign-off.

🤖 Generated with Claude Code

transport_common.h selects its x86 barriers with NVSHMEM_x86_64, but
CMake defines NVSHMEM_X86_64 (src/CMakeLists.txt,
src/modules/transport/CMakeLists.txt.in, src/device/CMakeLists.txt), and
nothing defines the lowercase name. On x86_64, MEM_BARRIER(),
STORE_BARRIER() and LOAD_BARRIER() therefore fall through to the generic
compiler-only barrier, so the STORE_BARRIER() calls before the ibdevx,
ibrc, staged, gpunetio and libfabric doorbell and flag writes never emit
sfence.

Use the macro that CMake defines, as the other x86 checks in the
transports already do.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: 0z5a <dezhen.lu@student.uni-tuebingen.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[x86_64] transport_common.h tests NVSHMEM_x86_64, but CMake defines NVSHMEM_X86_64, so STORE_BARRIER() is a compiler-only barrier on x86

1 participant