Address issues related to bytes vs. count

Author: davidbeckingsale

examples/cookbook/recipe_device_ipc.cpp

@@ -42,7 +42,8 @@ int main(int argc, char** argv)
 
     // Allocate device memory - only rank 0 will physically allocate
     // All other ranks will import via IPC
-    const size_t size = 1024 * sizeof(float);
+    constexpr std::size_t num_elements = 1024;
+    const size_t size = num_elements * sizeof(float);
     float* data = static_cast<float*>(ipc_allocator.allocate(size));
 
     std::cout << "Rank " << rank << ": Got device memory at " << data << std::endl;
@@ -60,7 +61,7 @@ int main(int argc, char** argv)
       }
 
       // Copy to device
-      umpire::copy(host_data, data, size);
+      umpire::copy(host_data, data, num_elements);
       host_allocator.deallocate(host_data);
     }
 
@@ -77,7 +78,7 @@ int main(int argc, char** argv)
     // All ranks can now access the data
     // Verify by copying a portion back to host
     float* value = static_cast<float*>(host_allocator.allocate(sizeof(float)));
-    umpire::copy(data + 1, value, sizeof(float));
+    umpire::copy(data + 1, value, 1);
 
     std::cout << "Rank " << rank << ": second value is " << *value << std::endl;
 

examples/multi_device.cpp

@@ -68,7 +68,7 @@ int main(int, char**)
   }
 #endif
 
-  umpire::copy(a, b, NUM_THREADS * sizeof(double));
+  umpire::copy(a, b, NUM_THREADS);
   b = static_cast<double*>(rm.move(b, rm.getAllocator("HOST")));
 
   UMPIRE_ASSERT(b[BLOCK_SIZE] == (BLOCK_SIZE * MULTIPLE) && "Error: incorrect value!");

examples/tutorial/tut_copy.cpp

@@ -16,7 +16,7 @@ void copy_data(double* source_data, std::size_t size, const std::string& destina
   double* dest_data = static_cast<double*>(dest_allocator.allocate(size * sizeof(double)));
 
   // _sphinx_tag_tut_copy_start
-  umpire::copy(source_data, dest_data, size * sizeof(double));
+  umpire::copy(source_data, dest_data, size);
   // _sphinx_tag_tut_copy_end
 
   std::cout << "Copied source data (" << source_data << ") to destination " << destination << " (" << dest_data << ")"

examples/tutorial/tut_reallocate.cpp

@@ -41,7 +41,7 @@ int main(int, char**)
     std::cout << "Reallocating data (" << data << ") to size " << REALLOCATED_SIZE << "...";
 
     // _sphinx_tag_tut_realloc_start
-    data = static_cast<double*>(umpire::reallocate(&data, REALLOCATED_SIZE * sizeof(double)));
+    data = umpire::reallocate(&data, REALLOCATED_SIZE);
     // _sphinx_tag_tut_realloc_end
 
     std::cout << "done.  Reallocated data (" << data << ")" << std::endl;

include/umpire/op/dispatch.hpp

@@ -289,9 +289,39 @@ struct op_caller {
 
 } // namespace op
 
+//------------------------------------------------------------------------------
+// Public API Semantics for Copy and Reallocate Operations
+//------------------------------------------------------------------------------
+//
+// The following functions use different semantics for size parameters based on
+// pointer type, matching standard C++ conventions:
+//
+// COPY OPERATIONS:
+//   - umpire::copy(T* src, T* dst, std::size_t len) for non-void T:
+//     len is a COUNT OF ELEMENTS (will be multiplied by sizeof(T) internally)
+//
+//   - umpire::copy(void* src, void* dst, std::size_t len):
+//     len is BYTES (used directly, no sizeof multiplication)
+//
+// REALLOCATE OPERATIONS:
+//   - umpire::reallocate(T** ptr, std::size_t new_size) for non-void T:
+//     new_size is a COUNT OF ELEMENTS (will be multiplied by sizeof(T) internally)
+//
+//   - umpire::reallocate(void** ptr, std::size_t new_size):
+//     new_size is BYTES (used directly, no sizeof multiplication)
+//
+// RATIONALE:
+//   This matches how detail::get_size<T>(count) works:
+//   - For void: returns count as-is (bytes)
+//   - For typed pointers: returns count * sizeof(T) (elements to bytes)
+//
+// This keeps the API consistent with C++ idioms where typed operations work
+// with element counts and void* operations work with byte counts.
+//------------------------------------------------------------------------------
+
 // Global operation implementations that use the op_caller
 template <typename T>
-auto copy(T* src, T* dst, std::size_t len)
+void copy(T* src, T* dst, std::size_t len)
 {
   op::op_caller<op::copy>::exec(src, dst, len);
 }
@@ -318,6 +348,10 @@ camp::resources::EventProxy<camp::resources::Resource> memset(T* src, int v, std
 template <typename T>
 inline T* reallocate(T** src, std::size_t size)
 {
+  // Handle nullptr specially - op_caller can't look up null in allocation map
+  if (*src == nullptr) {
+    return op::reallocate<resource::host_platform>::exec(src, size);
+  }
   return op::op_caller<op::reallocate>::exec(src, size);
 }
 
@@ -326,6 +360,10 @@ template <typename T>
 inline camp::resources::EventProxy<camp::resources::Resource> reallocate(T** src, std::size_t size,
                                                                          camp::resources::Resource& ctx)
 {
+  // Handle nullptr specially - op_caller can't look up null in allocation map
+  if (*src == nullptr) {
+    return op::reallocate<resource::host_platform>::exec(src, size, ctx);
+  }
   return op::op_caller<op::reallocate>::exec(src, size, ctx);
 }
 
@@ -444,11 +482,12 @@ T* op::reallocate<Src>::exec(T** ptr, std::size_t new_size)
   // Allocate new memory
   T* new_ptr = static_cast<T*>(allocator.allocate(new_bytes));
 
-  // Calculate copy size (minimum of old and new size)
-  std::size_t copy_size = (old_bytes > new_bytes) ? new_bytes : old_bytes;
+  // Calculate copy size in bytes (minimum of old and new size)
+  std::size_t copy_bytes = (old_bytes > new_bytes) ? new_bytes : old_bytes;
 
-  // Copy data from old to new location using auto-dispatch copy
-  umpire::copy(current_ptr, new_ptr, copy_size);
+  // Copy data using void* to pass bytes directly (avoids sizeof(T) multiplication in copy)
+  // Note: We cast to void* so that detail::get_size<void>(len) returns len as-is (bytes)
+  umpire::copy(static_cast<void*>(current_ptr), static_cast<void*>(new_ptr), copy_bytes);
 
   // Deallocate old memory
   allocator.deallocate(current_ptr);
@@ -507,11 +546,12 @@ camp::resources::EventProxy<camp::resources::Resource> op::reallocate<Src>::exec
   // Allocate new memory
   T* new_ptr = static_cast<T*>(allocator.allocate(new_bytes));
 
-  // Calculate copy size (minimum of old and new size)
-  std::size_t copy_size = (old_bytes > new_bytes) ? new_bytes : old_bytes;
+  // Calculate copy size in bytes (minimum of old and new size)
+  std::size_t copy_bytes = (old_bytes > new_bytes) ? new_bytes : old_bytes;
 
-  // Copy data from old to new location asynchronously using auto-dispatch copy
-  auto event = umpire::copy(current_ptr, new_ptr, copy_size, ctx);
+  // Copy data using void* to pass bytes directly (avoids sizeof(T) multiplication in copy)
+  // Note: We cast to void* so that detail::get_size<void>(len) returns len as-is (bytes)
+  auto event = umpire::copy(static_cast<void*>(current_ptr), static_cast<void*>(new_ptr), copy_bytes, ctx);
 
   // IMPORTANT: In a fully async implementation, we would need to chain the deallocation
   // to happen after the copy completes. However, since we don't have that mechanism yet,
@@ -571,11 +611,11 @@ void* op::reallocate<Src>::exec(void** ptr_ptr, std::size_t new_size)
   // Allocate new memory
   void* new_ptr = allocator.allocate(new_size);
 
-  // Calculate copy size (minimum of old and new size)
-  std::size_t copy_size = (old_size > new_size) ? new_size : old_size;
+  // Calculate copy size in bytes (minimum of old and new size)
+  std::size_t copy_bytes = (old_size > new_size) ? new_size : old_size;
 
-  // Copy data from old to new location using auto-dispatch copy
-  umpire::copy(current_ptr, new_ptr, copy_size);
+  // Copy data from old to new location (void* naturally uses bytes)
+  umpire::copy(static_cast<void*>(current_ptr), static_cast<void*>(new_ptr), copy_bytes);
 
   // Deallocate old memory
   allocator.deallocate(current_ptr);
@@ -629,11 +669,11 @@ camp::resources::EventProxy<camp::resources::Resource> op::reallocate<Src>::exec
   // Allocate new memory
   void* new_ptr = allocator.allocate(new_size);
 
-  // Calculate copy size (minimum of old and new size)
-  std::size_t copy_size = (old_size > new_size) ? new_size : old_size;
+  // Calculate copy size in bytes (minimum of old and new size)
+  std::size_t copy_bytes = (old_size > new_size) ? new_size : old_size;
 
-  // Copy data from old to new location asynchronously using auto-dispatch copy
-  auto event = umpire::copy(current_ptr, new_ptr, copy_size, ctx);
+  // Copy data from old to new location asynchronously (void* naturally uses bytes)
+  auto event = umpire::copy(static_cast<void*>(current_ptr), static_cast<void*>(new_ptr), copy_bytes, ctx);
 
   // Deallocate old memory
   allocator.deallocate(current_ptr);

include/umpire/op/host.hpp

@@ -73,5 +73,38 @@ struct memset<resource::host_platform> {
   }
 };
 
+template <>
+struct prefetch<resource::host_platform> {
+  /**
+   * @brief Host memory prefetch (no-op)
+   *
+   * Prefetch is a no-op for host memory since the CPU has direct access.
+   * This implementation exists for API compatibility and to avoid throwing errors.
+   *
+   * @tparam T Type of data being prefetched
+   * @param ptr Pointer to memory (unused)
+   * @param device Device ID (unused)
+   * @param len Number of bytes to prefetch (unused)
+   */
+  template <typename T>
+  static void exec(T* /*ptr*/, int /*device*/, std::size_t /*len*/) noexcept
+  {
+    // No-op: CPU already has direct access to host memory
+  }
+
+  /**
+   * @brief Asynchronous host memory prefetch (no-op)
+   *
+   * Returns a completed event immediately since this is a no-op.
+   */
+  template <typename T>
+  static camp::resources::EventProxy<camp::resources::Resource> exec(T* /*ptr*/, int /*device*/, std::size_t /*len*/,
+                                                                     camp::resources::Resource& resource) noexcept
+  {
+    // No-op: CPU already has direct access to host memory
+    return detail::make_completed_event(resource);
+  }
+};
+
 } // namespace op
 } // namespace umpire

tests/op/copy_test.cpp

@@ -145,4 +145,68 @@ TEST(Copy, ZeroSize)
   allocator.deallocate(dest_ptr);
 }
 
+// Test to validate that typed copy uses element counts, not bytes
+TEST(Copy, TypedCopySemantics)
+{
+  constexpr std::size_t num_elements = 100;
+
+  auto& rm = umpire::ResourceManager::getInstance();
+  auto allocator = rm.getAllocator("HOST");
+
+  // Allocate buffers for int*
+  int* source = static_cast<int*>(allocator.allocate(num_elements * sizeof(int)));
+  int* dest = static_cast<int*>(allocator.allocate(num_elements * sizeof(int)));
+
+  // Fill source with known pattern
+  for (std::size_t i = 0; i < num_elements; ++i) {
+    source[i] = static_cast<int>(i * 7 + 13);
+  }
+
+  // Zero out destination
+  std::memset(dest, 0, num_elements * sizeof(int));
+
+  // Copy using element count (not bytes)
+  umpire::copy(source, dest, num_elements);
+
+  // Verify exactly num_elements were copied
+  for (std::size_t i = 0; i < num_elements; ++i) {
+    ASSERT_EQ(dest[i], static_cast<int>(i * 7 + 13)) << "Element " << i << " not copied correctly";
+  }
+
+  // Cleanup
+  allocator.deallocate(source);
+  allocator.deallocate(dest);
+}
+
+// Test to validate void* copy uses bytes, not elements
+TEST(Copy, VoidVsTypedSemantics)
+{
+  constexpr std::size_t byte_count = 100;
+
+  auto& rm = umpire::ResourceManager::getInstance();
+  auto allocator = rm.getAllocator("HOST");
+
+  // Allocate as void*
+  void* void_src = allocator.allocate(byte_count);
+  void* void_dst = allocator.allocate(byte_count);
+
+  // Fill with byte pattern
+  std::memset(void_src, 0xAB, byte_count);
+  std::memset(void_dst, 0xCD, byte_count);
+
+  // Copy exact byte count
+  umpire::copy(void_src, void_dst, byte_count);
+
+  // Verify byte-for-byte copy
+  unsigned char* src_bytes = static_cast<unsigned char*>(void_src);
+  unsigned char* dst_bytes = static_cast<unsigned char*>(void_dst);
+  for (std::size_t i = 0; i < byte_count; ++i) {
+    ASSERT_EQ(dst_bytes[i], src_bytes[i]) << "Byte " << i << " not copied correctly";
+  }
+
+  // Cleanup
+  allocator.deallocate(void_src);
+  allocator.deallocate(void_dst);
+}
+
 // Additional tests for async copy operations could be added here

tests/op/memset_test.cpp

@@ -56,13 +56,12 @@ TEST(Memset, TypedMemsetInt)
     ptr[i] = static_cast<int>(i);
   }
 
-  // Use umpire::memset to zero out
-  umpire::memset(ptr, value, num_elements * sizeof(int));
+  // Use umpire::memset to zero out (pass element count for typed pointer)
+  umpire::memset(ptr, value, num_elements);
 
-  // Verify each byte is zero
-  unsigned char* byte_ptr = static_cast<unsigned char*>(static_cast<void*>(ptr));
-  for (std::size_t i = 0; i < num_elements * sizeof(int); ++i) {
-    ASSERT_EQ(byte_ptr[i], 0) << "Memset failed at byte " << i;
+  // Verify each int is zero
+  for (std::size_t i = 0; i < num_elements; ++i) {
+    ASSERT_EQ(ptr[i], 0) << "Memset failed at element " << i;
   }
 
   // Cleanup