[Parallel_libs-commits] [PATCH] D25701: Initial check-in of Acxxel (StreamExecutor renamed)

[Justin Lebar via Parallel_libs-commits]

jlebar added a comment.

Comments on cc file for header.



================
Comment at: acxxel/acxxel.cpp:32
+Expected<Platform *> getPlatform(const std::string &Name) {
+  std::string LowerCase(Name);
+  std::transform(LowerCase.begin(), LowerCase.end(), LowerCase.begin(),
----------------
Nit, Can we comment that this is case-insensitive?


================
Comment at: acxxel/acxxel.cpp:36
+#ifdef ACXXEL_ENABLE_CUDA
+  static Expected<std::unique_ptr<Platform>> MaybeCUDAPlatform = [] {
+    return createCUDAPlatform();
----------------
Using a unique_ptr here means that when the program shuts down, it will run the Platform's destructor.

This is usually something we avoid in large projects because these destructors are expensive and usually not actually useful.

If we wanted to avoid running the destructor here, we could just make this into an Expected<Platform*> and unwrap the unique_ptr. I am not sure if you want that, though -- there may be actual useful work you want to do on shutdown.


================
Comment at: acxxel/acxxel.cpp:78
+Stream::addCallback(std::function<void(Stream &, const Status &)> Callback) {
+  setStatus(ThePlatform->addStreamCallback(*this, Callback));
+  return *this;
----------------
std::move(Callback)


================
Comment at: acxxel/acxxel.h:295
+/// A program can be created by calling Platform::createProgramFromSource, and a
+/// Kernel can be created from a program by running Platform::createKernel.
+class Program {
----------------
jhen wrote:
> jlebar wrote:
> > What is the purpose of having separate program and kernel classes?  It seems like a kernel is just a program plus a name, so could we just merge the two?  If not, maybe we can briefly explain why they are separate concepts.
> The important difference is that a program can contain multiple kernels because the input source for the program can define multiple kernels. I added a comment to the `Program` class to explain this.
> 
> I don't think we want to merge `Program` with `Kernel` because, while it's true that a `Kernel` is basically just a program plus a name, it's not true that a `Program` is just a `Kernel` minus a name.
Oh, I see!  The comments clarify it for me, thanks.


================
Comment at: acxxel/acxxel.h:270
+
+  /// \name Synchronous device memory copies
+  ///
----------------
Let's expand a bit on this comment, specifically that these block the host until all previously-enqueued work on the stream plus the copy completes.


================
Comment at: acxxel/acxxel.h:280
+  template <typename DeviceSrcTy, typename DeviceDstTy>
+  Stream &copyDToD(DeviceSrcTy &&DeviceSrc, DeviceDstTy &&DeviceDst);
+
----------------
It might be worth calling these "syncCopyFoo" so there's no confusion about what they do.


https://reviews.llvm.org/D25701