guan404ming commented on code in PR #934:
URL: https://github.com/apache/mahout/pull/934#discussion_r2750487784
##########
qdp/qdp-core/src/lib.rs:
##########
@@ -492,7 +434,9 @@ impl QdpEngine {
{
crate::profile_scope!("GPU::Synchronize");
- sync_cuda_stream(stream, "CUDA stream synchronize
failed")?;
+ self.device.synchronize().map_err(|e| {
Review Comment:
`self.device.synchronize()` calls `cudaDeviceSynchronize`, which blocks
**all** streams on the device. The `_with_stream` variants should use
`sync_cuda_stream(stream, ...)` (`cudaStreamSynchronize`) to honor the caller's
stream. This is a behavioral regression for multi-stream scenarios. Same
applies to lines 489, 543, 732, 856, and 916.
##########
qdp/qdp-core/tests/gpu_ptr_encoding.rs:
##########
@@ -0,0 +1,277 @@
+//
Review Comment:
The main feature of this PR (basis GPU-pointer support) has no test
coverage. Please add at least:
- Happy path: `encode_from_gpu_ptr(..., "basis")` with a valid int64/usize
GPU buffer
- Happy path: `encode_batch_from_gpu_ptr(..., "basis")` with multi-sample
batch
- Validation: `input_len != 1` returns error for basis single
- Validation: `sample_size != 1` returns error for basis batch
##########
qdp/qdp-core/src/lib.rs:
##########
@@ -597,49 +597,52 @@ impl QdpEngine {
}
}
- /// Encode batch from existing GPU pointer on a specified CUDA stream.
+ /// Encode batch from existing GPU pointer with a specific CUDA stream.
+ ///
+ /// Same as [`encode_batch_from_gpu_ptr`](Self::encode_batch_from_gpu_ptr)
but uses the given
+ /// `stream` for kernel launches. Pass null for default stream.
///
/// # Safety
- /// In addition to the `encode_batch_from_gpu_ptr` requirements, the
stream pointer
- /// must remain valid for the duration of this call.
+ /// Same as
[`encode_batch_from_gpu_ptr`](Self::encode_batch_from_gpu_ptr). Additionally,
+ /// `stream` must be a valid CUDA stream on the same device as the engine,
or null.
#[cfg(target_os = "linux")]
pub unsafe fn encode_batch_from_gpu_ptr_with_stream(
&self,
- input_batch_d: *const f64,
+ input_batch_d: *const std::ffi::c_void,
num_samples: usize,
sample_size: usize,
num_qubits: usize,
encoding_method: &str,
- stream: *mut c_void,
+ stream: *mut std::ffi::c_void,
) -> Result<*mut DLManagedTensor> {
crate::profile_scope!("Mahout::EncodeBatchFromGpuPtr");
- if num_samples == 0 {
- return Err(MahoutError::InvalidInput(
- "Number of samples cannot be zero".into(),
- ));
- }
-
- if sample_size == 0 {
- return Err(MahoutError::InvalidInput(
- "Sample size cannot be zero".into(),
- ));
- }
-
- validate_cuda_input_ptr(&self.device, input_batch_d)?;
-
let state_len = 1usize << num_qubits;
let method = encoding_method.to_ascii_lowercase();
match method.as_str() {
"amplitude" => {
+ if num_samples == 0 {
Review Comment:
The `num_samples == 0` check is duplicated identically in the amplitude
(line 625), angle (line 744), and basis (line 864) arms. Consider extracting it
before the `match` to reduce duplication.
##########
qdp/qdp-core/src/lib.rs:
##########
@@ -726,7 +729,9 @@ impl QdpEngine {
{
crate::profile_scope!("GPU::Synchronize");
- sync_cuda_stream(stream, "CUDA stream synchronize
failed")?;
+ self.device
+ .synchronize()
Review Comment:
Inconsistent error message style within the same function. Line 438 uses
`"CUDA device synchronize failed: {:?}"` but here it's `"Sync failed: {:?}"`.
Use one consistent format across all sync error sites in this function.
--
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]
