[Parallel_libs-commits] [PATCH] D23138: [StreamExecutor] Add kernel types

[Jason Henline via Parallel_libs-commits]

jhen added a comment.

jlebar, thanks for your review on this. I'm sorry this patch has been so hard to read. :) Hopefully it's improved a lot now.


================
Comment at: streamexecutor/include/streamexecutor/Kernel.h:23
@@ +22,3 @@
+/// void Launch(
+///   const TypedKernel<ParameterTs...> &Kernel, ParamterTs... Arguments);
+/// \endcode
----------------
jlebar wrote:
> One thing I don't get is, why do we have to implement a Launch function like this at all?  Surely a better API would be to do
> 
>   Kernel.Launch(a1, a2, a3);
> 
> ?
It's because the real launch function is used in chaining like this

```
Stream.Initialize(...).thenMemcpyToDevice(...).thenLaunch(...).thenMemcpyToHost(...).thenBlock(...);
```

================
Comment at: streamexecutor/include/streamexecutor/Kernel.h:24
@@ +23,3 @@
+///   const TypedKernel<ParameterTs...> &Kernel, ParamterTs... Arguments);
+/// \endcode
+/// and the compiler will check that the user passes in arguments with types
----------------
jlebar wrote:
> I'm finding these code blocks pretty hard to read.  I'm sure they look great in doxygen, but...even under the highly generous estimate that as many people are going to read this in doxygen as in source, that's still a pretty bad situation.
> 
> What if we indented the code two or four spaces?
Sorry about that. I tried indenting them four spaces. I think it helps a lot.

================
Comment at: streamexecutor/lib/unittests/KernelTest.cpp:52
@@ +51,3 @@
+  // This is only safe to call once because the internal unique_ptr is no longer
+  // valid after the first call.
+  se::Expected<std::unique_ptr<se::KernelInterface>>
----------------
jlebar wrote:
> Not sure about this comment...it will just return null, but not do anything unsafe?  But maybe you want to return an error or something the second time?  Or assert()?
I added an assert and changed the comment to warn about the assert.

================
Comment at: streamexecutor/lib/unittests/KernelTest.cpp:85
@@ +84,3 @@
+  EXPECT_EQ(MockExecutor.getRaw(), Implementation);
+}
+
----------------
jlebar wrote:
> If I'm honest I have no idea what this is testing.
> 
> Maybe it is not in fact currently testing anything?  :)  If so I would almost prefer you wait to add it until we actually have something to test, so that I can better judge whether it's doing something reasonable.  But if there's a reason to do it this way, that's fine too.
Sorry for the confusing code. I added a comment to the test to describe what it's doing and I changed the test name to better describe what is going on.

>From my point of view, I think that it is really testing something, but I admit that it's testing something very simple. Let me know if you think the functionality it is testing is too simple to need a test.


https://reviews.llvm.org/D23138