Skip to content

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

Open
polarG wants to merge 1 commit into
mainfrom
dev/hongtaozhang/gpu-copy-bw-correctness
Open

Bugfix - Use volatile accesses for HIP zero-copy loads/stores in gpu-copy-bw#838
polarG wants to merge 1 commit into
mainfrom
dev/hongtaozhang/gpu-copy-bw-correctness

Conversation

@polarG

@polarG polarG commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

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).

The HIP branch of FetchULong2/StoreULong2 used plain cacheable accesses for zero-copy host-mapped memory, unlike the CUDA path which uses ld/st.volatile.global. This could serve stale data and fail the CheckBuf memcmp for cpu_to_gpu*_by_sm (observed on MI300X/ROCm 6.4.4). Use volatile accesses to match the CUDA path.
Copilot AI review requested due to automatic review settings July 29, 2026 21:15
@polarG
polarG requested a review from a team as a code owner July 29, 2026 21:15
@polarG polarG self-assigned this Jul 29, 2026
@polarG polarG added bug Something isn't working ROCm labels Jul 29, 2026

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 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.

Comment on lines +602 to +604
// 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);
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.02%. Comparing base (67298ae) to head (f57ce61).

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.12-unit-test 70.88% <ø> (ø)
cpu-python3.7-unit-test 70.31% <ø> (ø)

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.

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

Labels

bug Something isn't working ROCm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants