ryankert01 commented on code in PR #1399:
URL: https://github.com/apache/mahout/pull/1399#discussion_r3397540559
##########
qdp/qdp-core/src/gpu/encodings/amplitude.rs:
##########
@@ -877,6 +877,13 @@ impl AmplitudeEncoder {
{
crate::profile_scope!("GPU::NormValidationF32");
+ // The norm kernel ran on the caller's stream, but dtoh_sync_copy
+ // reads back on the default stream. Synchronize the caller's
stream
+ // first so the result is visible: with a non-blocking stream
(which
+ // does not implicitly order against the default stream) the
readback
+ // would otherwise race and observe the zero-initialized buffer.
This
+ // mirrors the single-sample path
(calculate_inv_norm_gpu_with_stream).
+ sync_cuda_stream(stream, "Norm stream synchronize failed (batch
f32)")?;
Review Comment:
Good fix — but worth being precise that this executes on the **CUDA path
too**, so it is a (correct) behavior change there, not just HIP enablement. The
norm kernel runs on the caller's non-blocking stream while `dtoh_sync_copy`
reads back on the legacy default stream, which non-blocking streams don't order
against — so the readback could race and observe the zeroed buffer on NVIDIA as
well. The second commit message states this accurately ("latent cross-stream
ordering hazard shared with the CUDA path"), but the PR description's "the
NVIDIA build is behavior-preserving (no functional change)" slightly oversells
it. Suggest flagging this as a CUDA-path bug fix in release notes.
Verified on NVIDIA at this head (RTX 3090 Ti): full suite 316 passed / 0
failed, including the dual-stream tests with `QDP_ENABLE_OVERLAP_TRACKING=1`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
