Bugfix - Use volatile accesses for HIP zero-copy loads/stores in gpu-copy-bw

[Hongtao Zhang (polarG)]

Description

The gpu-copy-bw micro-benchmark's SM-copy kernels use two helper functions, FetchULong2() and StoreULong2() (superbench/benchmarks/micro_benchmarks/gpu_copy_performance/gpu_copy.cu), to read/write 16-byte chunks from/to zero-copy host-mapped (or peer) memory. On the CUDA path these already use ld.volatile.global / st.volatile.global inline PTX — uncached, non-reorderable accesses, which is the correct pattern for memory that can be concurrently written by another device/host and isn't guaranteed to be cache-coherent through the normal load/store path. On the HIP path, the same functions instead did a plain, cacheable pointer dereference (v.x = p->x; v.y = p->y;), so the compiler was free to cache, reorder, or otherwise not treat these as true "read what's actually there right now" accesses.

This PR reinterprets the pointer as volatile ulong2* before dereferencing on the HIP path, forcing an uncached, non-reorderable access that matches the CUDA behavior.

Investigation context

This fix was originally written to explain a gpu-copy-bw:correctness failure (CheckBuf: Memory check failed on cpu_to_gpu0_by_sm_under_numa0) observed during a full SuperBench run on an MI300X/ROCm 6.4.4 host. Follow-up in-container investigation (rebuilding gpu_copy from source with an explicit --offload-arch=gfx942) showed that the actual failure in that run was caused by a missing AMDGPU_TARGETS build flag in the ROCm 6.4 Dockerfile, which made hipcc silently default to gfx906 when no GPU was present at docker build time — the resulting binary then ran wrong-architecture kernels on the gfx942 MI300X hardware and produced bad reads. That Dockerfile fix is tracked separately in #837.

With the correct gfx942 arch, the correctness suite passes 321/321 with or without this volatile change, so it was not the root cause of the specific failure observed. It is included here anyway as a genuine correctness hardening: the HIP path for zero-copy memory access should not silently diverge from the CUDA path's use of volatile/uncached semantics, and leaving it as a plain access is latent-bug-prone on any HIP toolchain/compiler version that becomes more aggressive about caching such reads.

Major Revision

• FetchULong2() (HIP branch): read through a const volatile ulong2 * instead of a plain const ulong2 *.
• StoreULong2() (HIP branch): write through a volatile ulong2 * instead of a plain ulong2 *.

Minor Revision

• None — single-file, four-line-of-intent change (10 insertions / 4 deletions after re-indentation).

Testing

• Rebuilt gpu_copy in-container on an 8x MI300X host with --offload-arch=gfx942; correctness suite (--check_data) passes 321/321 both with and without this change once the Dockerfile's AMDGPU_TARGETS fix (Dockerfile - Add ROCm6.4 dockerfile #837) is applied.
• No behavior change expected on CUDA (this only touches the #if defined(__HIP_PLATFORM_HCC__) || defined(__HCC__) || defined(__HIPCC__) branch).

[Copilot]

Pull request overview

This PR hardens the HIP implementation of gpu-copy-bw’s 16-byte zero-copy (host-mapped / peer) load/store helpers to better match the intent of the CUDA path by forcing volatile-qualified accesses.

Changes:

• Update HIP FetchULong2() to read via const volatile ulong2* instead of a plain pointer dereference.
• Update HIP StoreULong2() to write via volatile ulong2* instead of a plain pointer dereference.

Comments suppressed due to low confidence (1)

superbench/benchmarks/micro_benchmarks/gpu_copy_performance/gpu_copy.cu:622

• Similar to FetchULong2, the comment here states the CUDA volatile store is "uncached". Volatile primarily constrains compiler optimizations/reordering; cache behavior can be architecture/toolchain dependent. Rewording would keep the intent accurate.

    // Use a volatile access so the compiler does not cache/reorder this zero-copy write to
    // host-mapped (or peer) memory, matching the uncached st.volatile.global on the CUDA path.
    volatile ulong2 *vp = reinterpret_cast<volatile ulong2 *>(p);

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.02%. Comparing base (66564a5) to head (5d76241).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #838   +/-   ##
=======================================
  Coverage   86.02%   86.02%           
=======================================
  Files         103      103           
  Lines        7950     7950           
=======================================
  Hits         6839     6839           
  Misses       1111     1111           

Flag	Coverage Δ
cpu-python3.10-unit-test	70.88% <ø> (ø)
cpu-python3.12-unit-test	70.88% <ø> (ø)
cpu-python3.7-unit-test	70.31% <ø> (ø)
cuda-unit-test	83.95% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

superbench/benchmarks/micro_benchmarks/gpu_copy_performance/gpu_copy.cu:606

• The HIP path doesn’t need a reinterpret_cast to add volatile qualifiers; you can rely on the standard pointer qualification conversion. Also, the comment claims an "uncached" match to CUDA PTX, but volatile in C++ primarily constrains compiler optimizations (it doesn’t necessarily imply a hardware cache-bypass), so the wording is misleading.

    // Use a volatile access so the compiler does not cache/reorder this zero-copy read of
    // host-mapped (or peer) memory, matching the uncached ld.volatile.global on the CUDA path.
    const volatile ulong2 *vp = reinterpret_cast<const volatile ulong2 *>(p);
    v.x = vp->x;
    v.y = vp->y;

superbench/benchmarks/micro_benchmarks/gpu_copy_performance/gpu_copy.cu:624

• Same as the load side: the cast isn’t needed to add volatile, and the comment’s "uncached" claim is stronger than what C++ volatile guarantees on HIP (it constrains compiler reordering/caching, not necessarily the GPU cache hierarchy).

    // Use a volatile access so the compiler does not cache/reorder this zero-copy write to
    // host-mapped (or peer) memory, matching the uncached st.volatile.global on the CUDA path.
    volatile ulong2 *vp = reinterpret_cast<volatile ulong2 *>(p);
    vp->x = v.x;
    vp->y = v.y;

[Copilot]

Pull request overview

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

Suppressed comments (1)

superbench/benchmarks/micro_benchmarks/gpu_copy_performance/gpu_copy.cu:604

• The comment says this volatile dereference is "uncached" and matches ld.volatile.global semantics, but C++ volatile on HIP only constrains compiler optimizations (no caching/elision/reordering by the compiler) and does not guarantee hardware cache-bypass behavior. Consider rewording to avoid over-claiming the memory semantics.

    // Use a volatile access so the compiler does not cache/reorder this zero-copy read of
    // host-mapped (or peer) memory, matching the uncached ld.volatile.global on the CUDA path.
    const volatile ulong2 *vp = reinterpret_cast<const volatile ulong2 *>(p);

[Copilot]

Pull request overview

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

Suppressed comments (1)

superbench/benchmarks/micro_benchmarks/gpu_copy_performance/gpu_copy.cu:626

• Similar to FetchULong2, hardcoding unsigned long here assumes the scalar type of ulong2. Using auto keeps the implementation correct even if the underlying ulong2 definition changes, without affecting the volatile-store intent.

    volatile unsigned long *x_pointer = &p->x;
    volatile unsigned long *y_pointer = &p->y;
    *x_pointer = v.x;
    *y_pointer = v.y;

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.