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

[Justin Lebar via Parallel_libs-commits]

jlebar added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/Executor.h:93
@@ +92,3 @@
+                           llvm::MutableArrayRef<T> Dst, size_t ElementCount) {
+    if (ElementCount > Src.getElementCount())
+      return make_error("copying too many elements, " +
----------------
Nit, can we make these two inequalities have the same direction?

================
Comment at: streamexecutor/include/streamexecutor/PlatformInterfaces.h:160
@@ +159,3 @@
+  /// any host memory, not just registered host memory or host memory allocated
+  /// by allocateHostMemory.
+  virtual Error synchronousCopyD2H(const GlobalDeviceMemoryBase &DeviceSrc,
----------------
Should we specify the device synchronicity semantics for these calls (and also in Executor, I guess)?  The non-"synchronous" functions clearly are synchronous within a stream.  But for these I have no idea what they're supposed to do.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:111
@@ +110,3 @@
+                        const GlobalDeviceMemory<T> &DeviceSrc,
+                        size_t ElementCount, size_t SrcElementOffset = 0) {
+    if (ElementCount + SrcElementOffset > DeviceSrc.getElementCount())
----------------
I guess I expected that we'd match the Executor's interface, with the overloads and such.  But maybe you plan to do that in a separate patch?

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:177
@@ +176,3 @@
+               llvm::Twine(HostSrc.size()) +
+               " ,does not match device destination element count, " +
+               llvm::Twine(DeviceDst->getElementCount()));
----------------
comma placement


https://reviews.llvm.org/D23577