Bugfix - Use volatile accesses for HIP zero-copy loads/stores in gpu-copy-bw - #838
Open
polarG wants to merge 1 commit into
Open
Bugfix - Use volatile accesses for HIP zero-copy loads/stores in gpu-copy-bw#838polarG wants to merge 1 commit into
polarG wants to merge 1 commit into
Conversation
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.
Contributor
There was a problem hiding this comment.
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 viaconst volatile ulong2*instead of a plain pointer dereference. - Update HIP
StoreULong2()to write viavolatile 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 Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The
gpu-copy-bwmicro-benchmark's SM-copy kernels use two helper functions,FetchULong2()andStoreULong2()(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 useld.volatile.global/st.volatile.globalinline 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:correctnessfailure (CheckBuf: Memory check failedoncpu_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 (rebuildinggpu_copyfrom source with an explicit--offload-arch=gfx942) showed that the actual failure in that run was caused by a missingAMDGPU_TARGETSbuild flag in the ROCm 6.4 Dockerfile, which madehipccsilently default togfx906when no GPU was present atdocker buildtime — 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
gfx942arch, the correctness suite passes 321/321 with or without thisvolatilechange, 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 aconst volatile ulong2 *instead of a plainconst ulong2 *.StoreULong2()(HIP branch): write through avolatile ulong2 *instead of a plainulong2 *.Minor Revision
Testing
gpu_copyin-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'sAMDGPU_TARGETSfix (Dockerfile - Add ROCm6.4 dockerfile #837) is applied.#if defined(__HIP_PLATFORM_HCC__) || defined(__HCC__) || defined(__HIPCC__)branch).