Skip to content

Commit d97db73

Browse files
committed
[ROCm] QDP: order default-stream setup before forked-stream kernels
Restore the non-blocking forked stream on all platforms and fix the actual root cause of the Windows gfx1201 stream-test regression: a missing ordering between default-stream buffer setup and a kernel launched on a non-blocking stream that consumes it. Background: fork_default_stream creates a hipStreamNonBlocking stream to match cudarc and preserve the dual-stream copy/compute overlap. The encoders set up their input (htod) and output (alloc_zeros) buffers with the blocking shim copies, which run on the NULL/default stream, then launch the norm/encode kernels on the caller's forked stream. CUDA's legacy default stream is synchronizing, so on NVIDIA that setup is implicitly ordered before the forked-stream kernel reads the buffer. HIP's default stream is NOT synchronizing relative to a non-blocking stream, so the kernel raced the setup and read stale/zero data; the norm came back as the zero-initialized value and the result was wrong. A previous change worked around this by creating a blocking stream on Windows, which masked every such site at once but sacrificed the pipeline overlap and was attributed to a nonexistent "cache coherency gap" in the Windows runtime. A minimal HIP reproducer shows the runtime is correct: a kernel on a non-blocking stream that is properly ordered against the default stream (stream sync, device sync, or an event wait) reads back correct data on both the Adrenalin and ROCm runtimes; only the unordered case fails. The fix synchronizes the default stream at the end of the blocking alloc_zeros / htod copies (sync_default_stream), restoring the CUDA-equivalent ordering on every platform without touching the async pipeline (which uses async copies on explicit streams, not these blocking paths). The forked stream is non-blocking again everywhere. The symmetric readback hazard (a NULL-stream dtoh after a forked-stream kernel) is closed where it was still missing a sync: the f64/f32 batch norm-validation copies in amplitude.rs and the phase batch finiteness-probe copy in phase.rs now synchronize the caller's stream before reading back, matching the idiom the other encoders already use. This is arch-unified: correct on wave32 (gfx1100/gfx1151/gfx1201) and wave64 (gfx90a). On Linux the default stream is already synchronizing, so the added sync is a harmless no-op-cost ordering point. Authored with the assistance of Claude (Anthropic). Test Plan gfx1201 (RX 9070 XT, Windows 11, TheRock ROCm 7.14, HIP_VISIBLE_DEVICES=0): ``` export QDP_USE_HIP=1 QDP_HIP_ARCH_LIST=gfx1201 HIP_VISIBLE_DEVICES=0 cargo build -p qdp-core -p qdp-kernels --no-default-features --features hip cargo test -p qdp-core -p qdp-kernels --no-default-features --features hip -- --test-threads=1 ``` All suites pass, 0 failures. The three previously-regressed stream tests pass with the non-blocking stream restored: test_l2_norm_batch_kernel_stream, test_encode_from_gpu_ptr_f32_with_stream_non_default_success, test_encode_batch_from_gpu_ptr_f32_with_stream_success. qdp-core lib 77, gpu_ptr_encoding 68 (all 10 _with_stream variants pass), amplitude 21, angle 10, and all other GPU and non-GPU suites green.
1 parent 90b6006 commit d97db73

3 files changed

Lines changed: 49 additions & 18 deletions

File tree

qdp/qdp-core/src/gpu/encodings/amplitude.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -414,6 +414,12 @@ impl QuantumEncoder for AmplitudeEncoder {
414414
};
415415
{
416416
crate::profile_scope!("GPU::NormValidation");
417+
// The norm kernel ran on the caller's stream, but dtoh_sync_copy reads
418+
// back on the default stream. Synchronize the caller's stream first so
419+
// the result is visible: with a non-blocking stream (which does not
420+
// implicitly order against the default stream) the readback would
421+
// otherwise race and observe the zero-initialized buffer.
422+
sync_cuda_stream(stream, "Norm stream synchronize failed (batch)")?;
417423
let host_inv_norms = device
418424
.dtoh_sync_copy(&inv_norms_gpu)
419425
.map_err(|e| MahoutError::Cuda(format!("Failed to copy norms to host: {:?}", e)))?;
@@ -646,6 +652,12 @@ impl QuantumEncoder for AmplitudeEncoder {
646652
};
647653
{
648654
crate::profile_scope!("GPU::NormValidation_f32");
655+
// The norm kernel ran on the caller's stream, but dtoh_sync_copy reads
656+
// back on the default stream. Synchronize the caller's stream first so
657+
// the result is visible: with a non-blocking stream (which does not
658+
// implicitly order against the default stream) the readback would
659+
// otherwise race and observe the zero-initialized buffer.
660+
sync_cuda_stream(stream, "Norm stream synchronize failed (batch f32)")?;
649661
let host_inv_norms = device
650662
.dtoh_sync_copy(&inv_norms_gpu)
651663
.map_err(|e| MahoutError::Cuda(format!("Failed to copy norms to host: {:?}", e)))?;

qdp/qdp-core/src/gpu/encodings/phase.rs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,15 @@ impl QuantumEncoder for PhaseEncoder {
353353

354354
{
355355
crate::profile_scope!("GPU::PhaseFiniteValidationHostCopy");
356+
// The norm probe ran on the caller's stream, but dtoh_sync_copy reads
357+
// back on the default stream. Synchronize the caller's stream first so
358+
// the result is visible: with a non-blocking stream (which does not
359+
// implicitly order against the default stream) the readback would
360+
// otherwise race and observe the zero-initialized buffer.
361+
crate::gpu::cuda_sync::sync_cuda_stream(
362+
stream,
363+
"Phase validation norm stream synchronize failed (batch)",
364+
)?;
356365
let host_norms = device
357366
.dtoh_sync_copy(&phase_validation_buffer)
358367
.map_err(|e| {

qdp/qdp-kernels/src/device.rs

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,6 @@ mod hip {
5959
const HIP_MEMCPY_DEVICE_TO_HOST: u32 = 2;
6060
// hipStreamNonBlocking: the new stream does not implicitly synchronize with
6161
// the NULL/default stream, matching cudarc's fork_default_stream.
62-
// Used on Linux only; Windows uses a blocking stream (see fork_default_stream).
63-
#[cfg(target_os = "linux")]
6462
const HIP_STREAM_NON_BLOCKING: u32 = 1;
6563

6664
unsafe extern "C" {
@@ -71,10 +69,7 @@ mod hip {
7169
fn hipMemset(ptr: *mut c_void, value: i32, size: usize) -> hipError_t;
7270
fn hipMemcpy(dst: *mut c_void, src: *const c_void, size: usize, kind: u32) -> hipError_t;
7371
fn hipDeviceSynchronize() -> hipError_t;
74-
#[cfg(target_os = "linux")]
7572
fn hipStreamCreateWithFlags(stream: *mut *mut c_void, flags: u32) -> hipError_t;
76-
#[cfg(not(target_os = "linux"))]
77-
fn hipStreamCreate(stream: *mut *mut c_void) -> hipError_t;
7873
fn hipStreamDestroy(stream: *mut c_void) -> hipError_t;
7974
fn hipStreamSynchronize(stream: *mut c_void) -> hipError_t;
8075
}
@@ -92,6 +87,24 @@ mod hip {
9287
}
9388
}
9489

90+
/// Synchronize the NULL/default stream so its prior work is ordered before any
91+
/// other stream observes the affected buffers.
92+
///
93+
/// The blocking shim copies (htod/alloc_zeros) issue hipMemcpy/hipMemset on the
94+
/// default (NULL) stream. CUDA's legacy default stream is synchronizing, so on
95+
/// NVIDIA that work is implicitly ordered before a kernel launched on a forked
96+
/// non-blocking stream that reads the same buffer. HIP's default stream is NOT
97+
/// synchronizing relative to a hipStreamNonBlocking stream, so without this an
98+
/// encoder that sets up input/output on the default stream and then launches
99+
/// the norm/encode kernels on the caller's forked stream would race the setup
100+
/// (the kernel reads stale/zero data). A default-stream synchronize after the
101+
/// blocking copy restores the CUDA-equivalent ordering while preserving the
102+
/// dual-stream copy/compute overlap (which uses async copies on explicit
103+
/// streams, not these blocking paths).
104+
fn sync_default_stream() -> Result<(), DriverError> {
105+
unsafe { check(hipStreamSynchronize(std::ptr::null_mut())) }
106+
}
107+
95108
/// Marker: type is safe to byte-copy to/from the device. Mirrors
96109
/// `cudarc::driver::DeviceRepr`.
97110
///
@@ -305,6 +318,7 @@ mod hip {
305318
if bytes > 0 {
306319
unsafe {
307320
check(hipMemset(slice.raw_ptr(), 0, bytes))?;
321+
sync_default_stream()?;
308322
}
309323
}
310324
Ok(slice)
@@ -354,6 +368,7 @@ mod hip {
354368
bytes,
355369
HIP_MEMCPY_HOST_TO_DEVICE,
356370
))?;
371+
sync_default_stream()?;
357372
}
358373
}
359374
Ok(())
@@ -400,23 +415,18 @@ mod hip {
400415
self.bind()?;
401416
let mut stream: *mut c_void = std::ptr::null_mut();
402417
unsafe {
403-
// On Linux, use a non-blocking stream (HIP_STREAM_NON_BLOCKING=1) to
404-
// match cudarc: a blocking stream serializes H2D copies against the NULL
405-
// stream and defeats the dual-stream copy/compute overlap the pipeline
406-
// relies on.
407-
//
408-
// On Windows (TheRock ROCm 7.14), non-blocking stream writes are not
409-
// visible via hipMemcpy from the host after hipStreamSynchronize, due to
410-
// a cache coherency gap in the Windows HIP runtime. Use a blocking stream
411-
// to restore correct D2H readback semantics. The pipeline overlap
412-
// optimization is not available on Windows ROCm.
413-
#[cfg(target_os = "linux")]
418+
// Non-blocking stream (matches cudarc on both Linux and Windows): a
419+
// default/blocking stream would serialize H2D copies against the NULL
420+
// stream and defeat the dual-stream copy/compute overlap the pipeline
421+
// relies on. Because a non-blocking stream does not implicitly order
422+
// against the default stream, any host-visible readback (a default/NULL
423+
// stream dtoh copy) of results produced on this stream MUST first
424+
// synchronize this stream (wait_for / sync_cuda_stream); the encoders
425+
// already do so before every readback.
414426
check(hipStreamCreateWithFlags(
415427
&mut stream,
416428
HIP_STREAM_NON_BLOCKING,
417429
))?;
418-
#[cfg(not(target_os = "linux"))]
419-
check(hipStreamCreate(&mut stream))?;
420430
}
421431
Ok(CudaStream {
422432
stream,

0 commit comments

Comments
 (0)