[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:90
@@ +89,3 @@
+  // This protection prevents a user from creating a Stream with a nullptr
+  // implementation.
+  Stream(StreamExecutorKey SK, std::unique_ptr<StreamInterface> Implementation);
----------------
If that's the only point, could we just assert that Implementation is not null?

One advantage of getting rid of SEKey would be, that (maybe?) we could move StreamKey into Stream itself.  Right now we can't do that because you can't forward-declare a nested type, and so there's no way to have StreamKey inside Stream and SEKey inside SE and have them both depend on each other via #includes.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:123
@@ +122,3 @@
+                     const ParameterTs &... Arguments) {
+    auto ArgumentArray =
+        make_kernel_argument_pack<ParameterTs...>(Arguments...);
----------------
Discussed IRL, it sounds like the point is to pass references, not ownership.  So this is good.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:125
@@ +124,3 @@
+        make_kernel_argument_pack<ParameterTs...>(Arguments...);
+    return thenRawLaunch(BlockSize, GridSize, Kernel, ArgumentArray);
+  }
----------------
Sorry, I meant to add another comment.  But it's moot if we're taking const refs.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:217
@@ +216,3 @@
+  /// (enqueued up to this point in program execution) to complete.
+  ///
+  /// Returns true if there are no errors on the stream.
----------------
Hm.  Okay, I don't see a better way to do this at the moment.  If we had a more complex error class it would be worth worrying about, but as-is, it's fine.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:272
@@ +271,3 @@
+  /// If nullptr, there have been no errors in this stream.
+  std::unique_ptr<std::string> ErrorMessage;
+
----------------
Nit, I think you want llvm::optional<std::string>.


https://reviews.llvm.org/D23333