[Parallel_libs-commits] [PATCH] D23577: [StreamExecutor] Executor add synchronous methods

[Jason Henline via Parallel_libs-commits]

jhen added a comment.

In https://reviews.llvm.org/D23577#518094, @jlebar wrote:

> I had a thought about MemcpyD2H and friends last night.  Not to make you change it again, but it's kind of weird that the function name says "D2H" but the args are (host_ptr, device_ptr).  So long as it's called "memcpy", I think we probably should match memcpy's ordering, but what if we just called it "copy"?
>
> That might somewhat speak to your concern about users getting the order wrong.


Yes, the D2H, etc names were the reason for the original (src, dst) argument ordering, so I think there is a potential for confusion with the current ordering.

For an alternative solution that really helps the user know what they are doing, how about introducing different wrapper classes for src and dst and requiring the user to wrap the argument before passing it to the copy method:

  // Wrapper classes.
  template <typename T> class CopySrc;
  template <typename T> class CopyDst;
  
  // Copy function decl.
  template <typename T>
  Error synchronousCopy(CopyDst<T> Src, CopySrc<T> Dst, size_t ElementCount);
  
  // Helper functions "to" and "from" to construct wrapper instances.
  template <typename T> CopySrc<T> from(const GlobalDeviceMemory<T> &DeviceSrc, size_t Offset = 0);
  template <typename T> CopySrc<T> from(const T *HostSrc, size_t Offset = 0);
  template <typename T> CopyDst<T> to(GlobalDeviceMemory<T> DeviceDst, size_t Offset = 0);
  template <typename T> CopyDst<T> to(T *HostDst, size_t Offset = 0);
  
  // User code that calls a copy.
  Executor.synchronousCopy(to(DeviceMemory, DeviceElementOffset), from(HostPtr), ElementCount);

This seems to me like a pretty foolproof interface, and I like that it allows the user to specify the offset along with the pointer only if an offset is desired.

What do you think?


https://reviews.llvm.org/D23577