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

[Justin Lebar via Parallel_libs-commits]

jlebar added a comment.

Looks pretty good to me.


================
Comment at: streamexecutor/include/streamexecutor/Kernel.h:11
@@ +10,3 @@
+/// \file
+/// Types to represent device kernels (code compiled to run on a device).
+///
----------------
If we're being helpful, maybe s/on a device/on a GPU or other accelerator/.

================
Comment at: streamexecutor/include/streamexecutor/Kernel.h:14
@@ +13,3 @@
+/// The TypedKernel class is used to provide type safety to the StreamExecutor
+/// user API kernel launch methods, and the KernelBase class is used like a
+/// void* function pointer to perform unsafe operations inside StreamExecutor.
----------------
Too many adjectives in a row.  "user API's"

================
Comment at: streamexecutor/include/streamexecutor/Kernel.h:15
@@ +14,3 @@
+/// user API kernel launch methods, and the KernelBase class is used like a
+/// void* function pointer to perform unsafe operations inside StreamExecutor.
+///
----------------
s/unsafe/type-unsafe/?

================
Comment at: streamexecutor/include/streamexecutor/Kernel.h:18
@@ +17,3 @@
+/// With the kernel parameter types recorded in the TypedKernel template
+/// parameters, type safe kernel launch methods can be written with signatures
+/// like the following:
----------------
s/methods/functions/?

================
Comment at: streamexecutor/include/streamexecutor/Kernel.h:18
@@ +17,3 @@
+/// With the kernel parameter types recorded in the TypedKernel template
+/// parameters, type safe kernel launch methods can be written with signatures
+/// like the following:
----------------
jlebar wrote:
> s/methods/functions/?
Uber-nit, but "type-safe"?  The latter seems ~5x more common in my searching.

================
Comment at: streamexecutor/include/streamexecutor/Kernel.h:23
@@ +22,3 @@
+/// void Launch(
+///   const TypedKernel<ParameterTs...> &Kernel, ParamterTs... Arguments);
+/// \endcode
----------------
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);

?

================
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
----------------
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?

================
Comment at: streamexecutor/include/streamexecutor/Kernel.h:28
@@ +27,3 @@
+///
+/// The problem is that a TypedKernel template specialization with the right
+/// parameter types must be passed as the first argument to the Launch function,
----------------
Maybe s/The problem/A problem/ or something?

================
Comment at: streamexecutor/include/streamexecutor/Kernel.h:97
@@ +96,3 @@
+public:
+  KernelBase(KernelBase &&);
+  ~KernelBase();
----------------
In general if you implement a move (copy) constructor, you should implement a move (copy) assignment operator.

http://en.cppreference.com/w/cpp/language/rule_of_three

================
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>>
----------------
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()?

================
Comment at: streamexecutor/lib/unittests/KernelTest.cpp:85
@@ +84,3 @@
+  EXPECT_EQ(MockExecutor.getRaw(), Implementation);
+}
+
----------------
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.


https://reviews.llvm.org/D23138