[StreamExecutor] Add kernel types

[jhen]

Summary

Add StreamExecutor kernel types.

[jlebar]

Looks pretty good to me.

streamexecutor/include/streamexecutor/Kernel.h

11 ↗

(On Diff #66721)

If we're being helpful, maybe s/on a device/on a GPU or other accelerator/.

14 ↗

(On Diff #66721)

Too many adjectives in a row. "user API's"

15 ↗

(On Diff #66721)

s/unsafe/type-unsafe/?

18 ↗

(On Diff #66721)

s/methods/functions/?

18 ↗

(On Diff #66721)

Uber-nit, but "type-safe"? The latter seems ~5x more common in my searching.

23 ↗

(On Diff #66721)

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);

?

24 ↗

(On Diff #66721)

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?

28 ↗

(On Diff #66721)

Maybe s/The problem/A problem/ or something?

97 ↗

(On Diff #66721)

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

streamexecutor/lib/unittests/KernelTest.cpp

52 ↗

(On Diff #66721)

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

85 ↗

(On Diff #66721)

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.

[jhen]

• Respond to jlebar's comments

[jhen]

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.

streamexecutor/include/streamexecutor/Kernel.h

23 ↗

(On Diff #66721)

It's because the real launch function is used in chaining like this

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

24 ↗

(On Diff #66721)

Sorry about that. I tried indenting them four spaces. I think it helps a lot.

streamexecutor/lib/unittests/KernelTest.cpp

52 ↗

(On Diff #66721)

I added an assert and changed the comment to warn about the assert.

85 ↗

(On Diff #66721)

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.

[jhen]

Oh, and in response to our offline discussion about gmock. I realized that gmock is not used in LLVM, so I'm not going to use it either. So, I removed the comment that mentioned gmock and I'm now just defining my mock class without mentioning gmock.

[jprice]

streamexecutor/lib/Kernel.cpp

42 ↗

(On Diff #66749)

This needed a std::move() for me to build with GCC 5 on Ubuntu or Apple Clang on OS X.

[jlebar]

I'm sorry this patch has been so hard to read. :)

Oh, sorry if I gave you the wrong impression. I do a lot of reviews, and your patches are consistently in the top quintile, both for readability and just in general. This one is no exception!

Thank you for the fixes, this looks good to me (modulo the outstanding std::move issue).

streamexecutor/include/streamexecutor/Kernel.h

24 ↗

(On Diff #66749)

Ah, I see. Okay!

streamexecutor/lib/unittests/KernelTest.cpp

86 ↗

(On Diff #66749)

Okay, I get it now! Thanks. This is fine. Like you say it's simple, but that's not a problem.

[jhen]

• Fix bug found by jprice

streamexecutor/lib/Kernel.cpp

42 ↗

(On Diff #66749)

Thanks for spotting this jprice. I can confirm that it fails to build with GCC 4.8 on Ubuntu as well without the std::move(). I have now added the std::move() to prevent this error.

parallel-libs/trunk/streamexecutor/CMakeLists.txt

if(STREAM_EXECUTOR_STANDALONE)
     message(STATUS "Found LLVM ${LLVM_PACKAGE_VERSION}")
     message(STATUS "Using LLVMConfig.cmake in: ${LLVM_DIR}")
 
     include_directories(${LLVM_INCLUDE_DIRS})
     add_definitions(${LLVM_DEFINITIONS})
 
     # Find the libraries that correspond to the LLVM components
     # that we wish to use
-    llvm_map_components_to_libnames(llvm_libs support)
+    llvm_map_components_to_libnames(llvm_libs support symbolize)
 
     if(STREAM_EXECUTOR_UNIT_TESTS)
         enable_testing()
         find_package(GTest REQUIRED)
         include_directories(${GTEST_INCLUDE_DIRS})
         find_package(Threads REQUIRED)
     endif()
 else(NOT STREAM_EXECUTOR_STANDALONE)

parallel-libs/trunk/streamexecutor/include/streamexecutor/Interfaces.h

+//===-- Interfaces.h - Interfaces to platform-specific impls ----*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// Interfaces to platform-specific StreamExecutor type implementations.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef STREAMEXECUTOR_INTERFACES_H
+#define STREAMEXECUTOR_INTERFACES_H
+
+namespace streamexecutor {
+
+/// Methods supported by device kernel function objects on all platforms.
+class KernelInterface {
+  // TODO(jhen): Add methods.
+};
+
+// TODO(jhen): Add other interfaces such as Stream.
+
+} // namespace streamexecutor
+
+#endif // STREAMEXECUTOR_INTERFACES_H

parallel-libs/trunk/streamexecutor/include/streamexecutor/Kernel.h

+//===-- Kernel.h - StreamExecutor kernel types ------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// Types to represent device kernels (code compiled to run on GPU or other
+/// accelerator).
+///
+/// The TypedKernel class is used to provide type safety to the user API's
+/// launch functions, and the KernelBase class is used like a void* function
+/// pointer to perform type-unsafe operations inside StreamExecutor.
+///
+/// With the kernel parameter types recorded in the TypedKernel template
+/// parameters, type-safe kernel launch functions can be written with signatures
+/// like the following:
+/// \code
+///     template <typename... ParameterTs>
+///     void Launch(
+///       const TypedKernel<ParameterTs...> &Kernel, ParamterTs... Arguments);
+/// \endcode
+/// and the compiler will check that the user passes in arguments with types
+/// matching the corresponding kernel parameters.
+///
+/// A problem is that a TypedKernel template specialization with the right
+/// parameter types must be passed as the first argument to the Launch function,
+/// and it's just as hard to get the types right in that template specialization
+/// as it is to get them right for the kernel arguments.
+///
+/// With this problem in mind, it is not recommended for users to specialize the
+/// TypedKernel template class themselves, but instead to let the compiler do it
+/// for them. When the compiler encounters a device kernel function, it can
+/// create a TypedKernel template specialization in the host code that has the
+/// right parameter types for that kernel and which has a type name based on the
+/// name of the kernel function.
+///
+/// For example, if a CUDA device kernel function with the following signature
+/// has been defined:
+/// \code
+///     void Saxpy(float *A, float *X, float *Y);
+/// \endcode
+/// the compiler can insert the following declaration in the host code:
+/// \code
+///     namespace compiler_cuda_namespace {
+///     using SaxpyKernel =
+///         streamexecutor::TypedKernel<float *, float *, float *>;
+///     } // namespace compiler_cuda_namespace
+/// \endcode
+/// and then the user can launch the kernel by calling the StreamExecutor launch
+/// function as follows:
+/// \code
+///     namespace ccn = compiler_cuda_namespace;
+///     // Assumes Executor is a pointer to the StreamExecutor on which to
+///     // launch the kernel.
+///     //
+///     // See KernelSpec.h for details on how the compiler can create a
+///     // MultiKernelLoaderSpec instance like SaxpyKernelLoaderSpec below.
+///     Expected<ccn::SaxpyKernel> MaybeKernel =
+///         ccn::SaxpyKernel::create(Executor, ccn::SaxpyKernelLoaderSpec);
+///     if (!MaybeKernel) { /* Handle error */ }
+///     ccn::SaxpyKernel SaxpyKernel = *MaybeKernel;
+///     Launch(SaxpyKernel, A, X, Y);
+/// \endcode
+///
+/// With the compiler's help in specializing TypedKernel for each device kernel
+/// function (and generating a MultiKernelLoaderSpec instance for each kernel),
+/// the user can safely launch the device kernel from the host and get an error
+/// message at compile time if the argument types don't match the kernel
+/// parameter types.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef STREAMEXECUTOR_KERNEL_H
+#define STREAMEXECUTOR_KERNEL_H
+
+#include "streamexecutor/KernelSpec.h"
+#include "streamexecutor/Utils/Error.h"
+
+#include <memory>
+
+namespace streamexecutor {
+
+class KernelInterface;
+class StreamExecutor;
+
+/// The base class for device kernel functions.
+///
+/// This class has no information about the types of the parameters taken by the
+/// kernel, so it is analogous to a void* pointer to a device function.
+///
+/// See the TypedKernel class below for the subclass which does have information
+/// about parameter types.
+class KernelBase {
+public:
+  KernelBase(KernelBase &&) = default;
+  KernelBase &operator=(KernelBase &&) = default;
+  ~KernelBase();
+
+  /// Creates a kernel object from a StreamExecutor and a MultiKernelLoaderSpec.
+  ///
+  /// The StreamExecutor knows which platform it belongs to and the
+  /// MultiKernelLoaderSpec knows how to find the kernel code for different
+  /// platforms, so the combined information is enough to get the kernel code
+  /// for the appropriate platform.
+  static Expected<KernelBase> create(StreamExecutor *ParentExecutor,
+                                     const MultiKernelLoaderSpec &Spec);
+
+  const std::string &getName() const { return Name; }
+  const std::string &getDemangledName() const { return DemangledName; }
+
+  /// Gets a pointer to the platform-specific implementation of this kernel.
+  KernelInterface *getImplementation() { return Implementation.get(); }
+
+private:
+  KernelBase(StreamExecutor *ParentExecutor, const std::string &Name,
+             const std::string &DemangledName,
+             std::unique_ptr<KernelInterface> Implementation);
+
+  StreamExecutor *ParentExecutor;
+  std::string Name;
+  std::string DemangledName;
+  std::unique_ptr<KernelInterface> Implementation;
+
+  KernelBase(const KernelBase &) = delete;
+  KernelBase &operator=(const KernelBase &) = delete;
+};
+
+/// A device kernel function with specified parameter types.
+template <typename... ParameterTs> class TypedKernel : public KernelBase {
+public:
+  TypedKernel(TypedKernel &&) = default;
+  TypedKernel &operator=(TypedKernel &&) = default;
+
+  /// Parameters here have the same meaning as in KernelBase::create.
+  static Expected<TypedKernel> create(StreamExecutor *ParentExecutor,
+                                      const MultiKernelLoaderSpec &Spec) {
+    auto MaybeBase = KernelBase::create(ParentExecutor, Spec);
+    if (!MaybeBase) {
+      return MaybeBase.takeError();
+    }
+    TypedKernel Instance(std::move(*MaybeBase));
+    return std::move(Instance);
+  }
+
+private:
+  TypedKernel(KernelBase &&Base) : KernelBase(std::move(Base)) {}
+
+  TypedKernel(const TypedKernel &) = delete;
+  TypedKernel &operator=(const TypedKernel &) = delete;
+};
+
+} // namespace streamexecutor
+
+#endif // STREAMEXECUTOR_KERNEL_H

parallel-libs/trunk/streamexecutor/include/streamexecutor/KernelSpec.h

 /// so passing in specs with different kernel names can cause the program to
 /// abort.
 ///
 /// This interface is prone to errors, so it is better to leave
 /// MultiKernelLoaderSpec creation and initialization to the compiler rather
 /// than doing it by hand.
 class MultiKernelLoaderSpec {
 public:
+  std::string getKernelName() const {
+    if (TheKernelName) {
+      return *TheKernelName;
+    }
+    return "";
+  }
+
   // Convenience getters for testing whether these platform variants have
   // kernel loader specifications available.
 
   bool hasCUDAPTXInMemory() const { return TheCUDAPTXInMemorySpec != nullptr; }
   bool hasCUDAFatbinInMemory() const {
     return TheCUDAFatbinInMemorySpec != nullptr;
   }
   bool hasOpenCLTextInMemory() const {

parallel-libs/trunk/streamexecutor/include/streamexecutor/StreamExecutor.h

+//===-- StreamExecutor.h - The StreamExecutor class -------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// The StreamExecutor class which represents a single device of a specific
+/// platform.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef STREAMEXECUTOR_STREAMEXECUTOR_H
+#define STREAMEXECUTOR_STREAMEXECUTOR_H
+
+#include "streamexecutor/Utils/Error.h"
+
+namespace streamexecutor {
+
+class KernelInterface;
+
+class StreamExecutor {
+public:
+  /// Gets the kernel implementation for the underlying platform.
+  virtual Expected<std::unique_ptr<KernelInterface>>
+  getKernelImplementation(const MultiKernelLoaderSpec &Spec) {
+    // TODO(jhen): Implement this.
+    return nullptr;
+  }
+
+  // TODO(jhen): Add other methods.
+};
+
+} // namespace streamexecutor
+
+#endif // STREAMEXECUTOR_STREAMEXECUTOR_H

parallel-libs/trunk/streamexecutor/lib/CMakeLists.txt

 add_library(
     utils
     OBJECT
     Utils/Error.cpp)
 
 add_library(
     streamexecutor
     $<TARGET_OBJECTS:utils>
+    Kernel.cpp
     KernelSpec.cpp)
+target_link_libraries(streamexecutor ${llvm_libs})
 
 if(STREAM_EXECUTOR_UNIT_TESTS)
     add_subdirectory(unittests)
 endif()

parallel-libs/trunk/streamexecutor/lib/Kernel.cpp

+//===-- Kernel.cpp - General kernel implementation ------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// This file contains the implementation details for kernel types.
+///
+//===----------------------------------------------------------------------===//
+
+#include "streamexecutor/Kernel.h"
+#include "streamexecutor/Interfaces.h"
+#include "streamexecutor/StreamExecutor.h"
+
+#include "llvm/DebugInfo/Symbolize/Symbolize.h"
+
+namespace streamexecutor {
+
+KernelBase::KernelBase(StreamExecutor *ParentExecutor, const std::string &Name,
+                       const std::string &DemangledName,
+                       std::unique_ptr<KernelInterface> Implementation)
+    : ParentExecutor(ParentExecutor), Name(Name), DemangledName(DemangledName),
+      Implementation(std::move(Implementation)) {}
+
+KernelBase::~KernelBase() = default;
+
+Expected<KernelBase> KernelBase::create(StreamExecutor *ParentExecutor,
+                                        const MultiKernelLoaderSpec &Spec) {
+  auto MaybeImplementation = ParentExecutor->getKernelImplementation(Spec);
+  if (!MaybeImplementation) {
+    return MaybeImplementation.takeError();
+  }
+  std::string Name = Spec.getKernelName();
+  std::string DemangledName =
+      llvm::symbolize::LLVMSymbolizer::DemangleName(Name, nullptr);
+  KernelBase Instance(ParentExecutor, Name, DemangledName,
+                      std::move(*MaybeImplementation));
+  return std::move(Instance);
+}
+
+} // namespace streamexecutor

parallel-libs/trunk/streamexecutor/lib/unittests/CMakeLists.txt

 add_executable(
+    kernel_test
+    KernelTest.cpp)
+target_link_libraries(
+    kernel_test
+    streamexecutor
+    ${GTEST_BOTH_LIBRARIES}
+    ${CMAKE_THREAD_LIBS_INIT})
+add_test(KernelTest kernel_test)
+
+add_executable(
     kernel_spec_test
     KernelSpecTest.cpp)
 target_link_libraries(
     kernel_spec_test
     streamexecutor
     ${GTEST_BOTH_LIBRARIES}
     ${CMAKE_THREAD_LIBS_INIT})
 add_test(KernelSpecTest kernel_spec_test)

parallel-libs/trunk/streamexecutor/lib/unittests/KernelTest.cpp

+//===-- KernelTest.cpp - Tests for Kernel objects -------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+///
+/// \file
+/// This file contains the unit tests for the code in Kernel.
+///
+//===----------------------------------------------------------------------===//
+
+#include <cassert>
+
+#include "streamexecutor/Interfaces.h"
+#include "streamexecutor/Kernel.h"
+#include "streamexecutor/KernelSpec.h"
+#include "streamexecutor/StreamExecutor.h"
+
+#include "llvm/ADT/STLExtras.h"
+
+#include "gtest/gtest.h"
+
+namespace {
+
+namespace se = ::streamexecutor;
+
+// A StreamExecutor that returns a dummy KernelInterface.
+//
+// During construction it creates a unique_ptr to a dummy KernelInterface and it
+// also stores a separate copy of the raw pointer that is stored by that
+// unique_ptr.
+//
+// The expectation is that the code being tested will call the
+// getKernelImplementation method and will thereby take ownership of the
+// unique_ptr, but the copy of the raw pointer will stay behind in this mock
+// object. The raw pointer copy can then be used to identify the unique_ptr in
+// its new location (by comparing the raw pointer with unique_ptr::get), to
+// verify that the unique_ptr ended up where it was supposed to be.
+class MockStreamExecutor : public se::StreamExecutor {
+public:
+  MockStreamExecutor()
+      : Unique(llvm::make_unique<se::KernelInterface>()), Raw(Unique.get()) {}
+
+  // Moves the unique pointer into the returned se::Expected instance.
+  //
+  // Asserts that it is not called again after the unique pointer has been moved
+  // out.
+  se::Expected<std::unique_ptr<se::KernelInterface>>
+  getKernelImplementation(const se::MultiKernelLoaderSpec &) override {
+    assert(Unique && "MockStreamExecutor getKernelImplementation should not be "
+                     "called more than once");
+    return std::move(Unique);
+  }
+
+  // Gets the copy of the raw pointer from the original unique pointer.
+  const se::KernelInterface *getRaw() const { return Raw; }
+
+private:
+  std::unique_ptr<se::KernelInterface> Unique;
+  const se::KernelInterface *Raw;
+};
+
+// Test fixture class for typed tests for KernelBase.getImplementation.
+//
+// The only purpose of this class is to provide a name that types can be bound
+// to in the gtest infrastructure.
+template <typename T> class GetImplementationTest : public ::testing::Test {};
+
+// Types used with the GetImplementationTest fixture class.
+typedef ::testing::Types<se::KernelBase, se::TypedKernel<>,
+                         se::TypedKernel<int>>
+    GetImplementationTypes;
+
+TYPED_TEST_CASE(GetImplementationTest, GetImplementationTypes);
+
+// Tests that the kernel create functions properly fetch the implementation
+// pointers for the kernel objects they construct from the passed-in
+// StreamExecutor objects.
+TYPED_TEST(GetImplementationTest, SetImplementationDuringCreate) {
+  se::MultiKernelLoaderSpec Spec;
+  MockStreamExecutor MockExecutor;
+
+  auto MaybeKernel = TypeParam::create(&MockExecutor, Spec);
+  EXPECT_TRUE(static_cast<bool>(MaybeKernel));
+  se::KernelInterface *Implementation = MaybeKernel->getImplementation();
+  EXPECT_EQ(MockExecutor.getRaw(), Implementation);
+}
+
+} // namespace