[Parallel_libs-commits] [PATCH] D23333: [StreamExecutor] Add basic Stream operations

[Justin Lebar via Parallel_libs-commits]

jlebar added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:17
@@ +16,3 @@
+/// given device and then use that StreamExecutor to create a Stream instance.
+/// The Stream instance  will perform its work on the device managed by the
+/// StreamExecutor that created it.
----------------
Extra space

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:21
@@ +20,3 @@
+/// The various "then" methods of the Stream object, such as thenMemcpyH2D and
+/// thenLaunch, may be used to enqueue work on the Stream and the
+/// blockHostUntilDone() method may be used to block the host code until the
----------------
Missing a comma before "and".

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:25
@@ +24,3 @@
+///
+/// Multiple Stream instances can be created for the same StreamExecutor and
+/// this will allow several independent streams of computation can be performed
----------------
, and

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:26
@@ +25,3 @@
+/// Multiple Stream instances can be created for the same StreamExecutor and
+/// this will allow several independent streams of computation can be performed
+/// simultaneously on a single device.
----------------
to be

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:27
@@ +26,3 @@
+/// this will allow several independent streams of computation can be performed
+/// simultaneously on a single device.
+///
----------------
Perhaps: Multiple Stream instances can be created for the same StreamExecutor.  This allows ...

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:72
@@ +71,3 @@
+  StreamExecutorKey &operator=(const StreamExecutorKey &) = default;
+};
+
----------------
This is intriguing...

Is the idea that you don't want to make SE a friend of Stream, because that's too much of a layering violation?

I wonder if you could make SEKey a (private?) type inside Stream.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:109
@@ +108,3 @@
+      return Error::success();
+    }
+  };
----------------
LLVM style is to leave off curly braces where possible.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:113
@@ +112,3 @@
+  /// Entrains onto the stream of operations a kernel launch with the given
+  /// arguments for the invocation.
+  ///
----------------
Maybe s/ for the invocation//?

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:122
@@ +121,3 @@
+                     const TypedKernel<Params...> &Kernel,
+                     Params... Arguments) {
+    auto ArgumentArray = make_kernel_argument_pack(Arguments...);
----------------
Do you want `Params&&...` and `std::forward(Arguments...)` below?

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:124
@@ +123,3 @@
+    auto ArgumentArray = make_kernel_argument_pack(Arguments...);
+    return thenRawLaunch(BlockSize, GridSize, Kernel, ArgumentArray);
+  }
----------------
Do we want `std::move(ArgumentArray)`?  (At the moment it doesn't make sense because thenRawLaunch takes a const ref to the pack, but see below...)

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:132
@@ +131,3 @@
+  /// StreamExecutor::hostMemoryAllocate or otherwise allocated and then
+  /// registered with StreamExecutor::hostMemoryRegister.
+  template <typename T>
----------------
StreamExecutor::registerHostMemory might be a better name, but we can cross that bridge.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:136
@@ +135,3 @@
+                        llvm::MutableArrayRef<T> HostDst, bool FullCopy = true,
+                        size_t ElementCount = 0) {
+    if (FullCopy) {
----------------
These default args seem weird...would this be better as two functions, one that does a full copy and one that takes an ElementCount?

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:203
@@ +202,3 @@
+  /// Blocks the host code, waiting for the operations entrained on the stream
+  /// (enqueued to this point in program execution) to complete.
+  ///
----------------
"up to this point"?

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:216
@@ +215,3 @@
+      if (ErrorMessage == nullptr) {
+        ErrorMessage =
+            llvm::make_unique<std::string>(consumeAndGetMessage(std::move(E)));
----------------
Could we store an `Error` internally, instead of a string?

================
Comment at: streamexecutor/include/streamexecutor/StreamExecutor.h:57
@@ -33,2 +56,3 @@
 
-  // TODO(jhen): Add other methods.
+  Expected<std::unique_ptr<Stream>> createStream() {
+    // TODO(jhen): Get the real implementation pointer.
----------------
It looks like Stream is concrete and movable, so can we get rid of the unique_ptr?

================
Comment at: streamexecutor/lib/Stream.cpp:36
@@ +35,3 @@
+  return *this;
+}
+
----------------
So...it's illegal to move a stream if there are any outstanding operations?  (Among other reasons, we're not taking a mutex here.)  Maybe we should have a comment.

================
Comment at: streamexecutor/lib/Stream.cpp:38
@@ +37,3 @@
+
+Stream::~Stream() { consumeError(Parent->deallocateStream(this)); }
+
----------------
Is it important for us to have an API that would let consumers capture this error, if they wanted to?


https://reviews.llvm.org/D23333