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

[Justin Lebar via Parallel_libs-commits]

jlebar accepted this revision.
jlebar added a comment.
This revision is now accepted and ready to land.

I think this is officially unwieldy.  Which I accept the blame for.  :)

It looks good to me, and I think if we want to change things, it will probably be a lot easier to do so once this is in.


================
Comment at: streamexecutor/include/streamexecutor/PlatformInterfaces.h:160
@@ +159,3 @@
+  /// by allocateHostMemory. Also causes any device work enqueued after this
+  /// call to block until this call completes.
+  virtual Error synchronousCopyD2H(const GlobalDeviceMemoryBase &DeviceSrc,
----------------
Ah, but there can be no device work enqueued after the call starts but before it completes because this is host-synchronous!  (Unless you can touch Streams on multiple host threads?)

So maybe the thing to say is simply that this does not block any ongoing device calls?

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:109
@@ +108,3 @@
+  /// If the Src memory was not created by allocateHostMemory or registered with
+  /// registerHostMemory, then the copy operation my cause the host and device
+  /// to block until the copy operation is completed.
----------------
may

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:353
@@ +352,3 @@
+  /// GlobalDeviceMemorySlice<T>) but the source and destination are
+  /// GlobalDeviceMemory<T> rather than a GlobalDeviceMemorySlice<T>.
+  template <typename T>
----------------
This review is getting so long I'm tempted to say it will be easier to check this in as-is and edit things like this later.  But I think it would be nice to consider whether there's a way to reduce the comment copypasta -- there's a lot of it, and I think it obscures the API.  I think in an earlier patch you did some doxygen "function group" magic?

Again, maybe better to save this for later.


https://reviews.llvm.org/D23577