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

[Jason Henline via Parallel_libs-commits]

jhen added inline comments.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:72
@@ +71,3 @@
+  StreamExecutorKey &operator=(const StreamExecutorKey &) = default;
+};
+
----------------
jlebar wrote:
> 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.
> Is the idea that you don't want to make SE a friend of Stream, because that's too much of a layering violation?

Yes, I thought it would be nice to clarify the interface between StreamExecutor and Stream. Maybe this is overkill, but I thought it seemed lightweight enough to be OK.

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

I put SEKey into Stream, but I don't know if there is any way to make it a private type. I think StreamExecutor has to be able to see the definition so it can create an instance of SEKey, but maybe I'm missing something.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:122
@@ +121,3 @@
+                     const TypedKernel<Params...> &Kernel,
+                     Params... Arguments) {
+    auto ArgumentArray = make_kernel_argument_pack(Arguments...);
----------------
jlebar wrote:
> Do you want `Params&&...` and `std::forward(Arguments...)` below?
Ugh, thanks for catching that ugly copy. I changed the arguments to `const Params &... Arguments` instead of `Params &&...` for the following horrifying reason:

When I tried to use `Params &&...`, I got a compiler error when I tried to call `thenLaunch` with one `int` argument and one `GlobalDeviceMemory<int>` argument. The error said the compiler could not infer the types for `Params` because the `TypedKernel<Params...>` argument made it infer `int, GlobalDeviceMemory<int>` and the `Params &&...` argument made it infer `int, GlobalDeviceMemory<int> &`.

So, the `&&` seems to make the compiler infer a reference type for some arguments and that conflicts with the types from `TypedKernel<Params...>`.

Actually, I think const ref is the right signature for this function anyway, so I don't think we need to figure out a solution for the conflicting type inference. What do you think?

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:124
@@ +123,3 @@
+    auto ArgumentArray = make_kernel_argument_pack(Arguments...);
+    return thenRawLaunch(BlockSize, GridSize, Kernel, ArgumentArray);
+  }
----------------
jlebar wrote:
> 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...)
Maybe I missed it, but I didn't see the comment below about thenRawLaunch. I didn't change anything for now until we clear up the confusion.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:136
@@ +135,3 @@
+                        llvm::MutableArrayRef<T> HostDst, bool FullCopy = true,
+                        size_t ElementCount = 0) {
+    if (FullCopy) {
----------------
jlebar wrote:
> These default args seem weird...would this be better as two functions, one that does a full copy and one that takes an ElementCount?
Thanks for the idea. I was struggling with that design. Multiple functions is much nicer.

================
Comment at: streamexecutor/include/streamexecutor/Stream.h:216
@@ +215,3 @@
+      if (ErrorMessage == nullptr) {
+        ErrorMessage =
+            llvm::make_unique<std::string>(consumeAndGetMessage(std::move(E)));
----------------
jlebar wrote:
> Could we store an `Error` internally, instead of a string?
The problem I had with trying to use an `Error` is that the method to get out the error string destroys the error object. This is a side-effect of the way LLVM `Error` enforces the law that users cannot ignore their errors. So it looked like I was going to have to keep destroying and rebuilding the saved `Error` every time somebody queried it.

================
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.
----------------
jlebar wrote:
> It looks like Stream is concrete and movable, so can we get rid of the unique_ptr?
Oops, I was experimenting with moving streams and forgot to delete the stuff. Streams shouldn't be movable, so we'll stick with a pointer here.

================
Comment at: streamexecutor/lib/Stream.cpp:36
@@ +35,3 @@
+  return *this;
+}
+
----------------
jlebar wrote:
> 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.
Sorry I forgot to delete this garbage before. It's gone now and Streams are no longer movable.

================
Comment at: streamexecutor/lib/Stream.cpp:38
@@ +37,3 @@
+
+Stream::~Stream() { consumeError(Parent->deallocateStream(this)); }
+
----------------
jlebar wrote:
> Is it important for us to have an API that would let consumers capture this error, if they wanted to?
I think we can do that in a sane way. I change `StreamExecutor::deallocateStream` to return void and added a comment that says it will record an error if it fails. I left it as a TODO for now to design the interface for fetching stream destruction errors.


https://reviews.llvm.org/D23333