Change how users access OpenCL kernels

[SteveBronder]

Checklist

• Math issue Seperate OpenCL kernel access into it's own class #973

• Copyright holder: (fill in copyright holder information)

  - Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
  - Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
  

Rok Češnovar and Erik Štrumbelj (Faculty of Computer and Information Science, University of Ljubljana)
Steve Bronder

• the code base is stable

  • all unit tests pass
  • continuous integration passes

• the changes are maintainable

  • the code design is idiomatic C++ or there's a good reason it's not
  • please include appropriate documentation

• the changes are tested

• the changes adhere to the Math library's C++ standards

Summary

Easier Access To Kernels

Creates a kernel_cl_base singleton to manage the compiled kernels and a kernel_cl friend adapter class to give users access to kernels along with helper functions. Currently, kernels are compiled from within the opencl_context_base class. The GPU team is concerned that this class is becoming too bulky and so we created a new kernel_cl class that users can utilize to access kernels. Currently users run something like the following to compile and send arguments over to an OpenCL kernel.

cl::Kernel kernel = opencl_context.get_kernel("add");
opencl_context.set_kernel_args(kernel, C.buffer(), A.buffer(), B.buffer(),
                                   A.rows(), A.cols());

Users can now call the following

auto add_kernel = kernel_cl.add(C.buffer(), A.buffer(), B.buffer(), A.rows(), A.cols());

Remove Kernel Groups

This PR also removes the kernel groups such that each kernel is now compiled into it's own program. Originally the OpenCL context would compile kernels which were grouped together (IE 'cholesky', 'checks' ,etc.) into a single program with the idea being that if multiple kernels are used together it's smarter to compile all of them at once. While compiling multiple kernels into a single program is faster, as we add more kernels how we choose to label and group them would end up being difficult and too heuristic. When running some tests it did not seem like compiling the kernels separately produced a noticeable amount of overhead wrt the life of the executable.

Tests

The kernel_cl test checks whether the transpose kernel can be compiled. Code to run the test below

echo STAN_OPENCL=true>> make/local
echo OPENCL_PLATFORM_ID=0>> make/local
echo OPENCL_DEVICE_ID=0>> make/local
make test/unit/math/gpu/kernel_cl_test && ./test/unit/math/gpu/kernel_cl_test

Side Effects

None

Additional Notes

Leave additional notes for the code reviewer here.

[syclik]

Please create a new issue. It helps us manage this stuff; if there's a single issue per pull request, we can use "fixes #___" to automatically link and close issues.

[syclik]

whoa. I just read the description and that's great. what's with the naming? why kernel_cl_base?

If I'm understanding correctly, the constructor to kernel_cl looks up the compiled kernel by string?

[rok-cesnovar]

We are following the same naming convention we setup in the opencl_context (opencl_context_base is the singleton, opencl_context is the friend class).

kernel_cl checks if a kernel with the provided name has been compiled. If yes, it sets the member that holds the compiled kernel. If it wasnt compiled yet, it compiles it first.

One side effect of this PR is also that it organizes how we treat the kernel parameters (like LOWER, UPPER,...) a lot better.

[SteveBronder]

Along with what Rok mentioned above, I want to make a PR soon that changes all of the names from *_gpu_* to *_cl_*. I think it looks nicer and makes more sense. I'll file an issue soon describing the naming change in more detail

[seantalts]

Can we also make it so that the cl files are not string literals but would actually be readable by an IDE? :D

Also I'm happy to review GPU stuff - going through a workshop with Rok and Erik now on it and it's really cool. Let me know if I can take this off your plate, @syclik

[syclik]

@seantalts, please and thank you! I've been buried trying to get the make stuff to work consistently. (It's working through Math and Stan now! Now onto CmdStan.)

[SteveBronder]

Can we also make it so that the cl files are not string literals but would actually be readable by an IDE?

@seantalts I would absolutely love this but am having a hard time figuring out how to do it. When we tried reading them in with something like an ifstream we would get the instantiation fiasco error. Also wasn't sure how to specify the file locations since we are a header library.

The only other way I've seen to do it is via stringify like this stackoverflow Q. Though when I tried this it also threw up a bunch of errors, though maybe this is worth a second look

If you know a better way to do it then I'm 100% about it!

Also I'm happy to review GPU stuff

!! V appreciated !!

[seantalts]

Also just a note for those following this thread: some discussion has continued on the proposal above here: bstatcomp#7

[syclik]

Without digging through notes, one of the things was the use of forward declarations and implementation somewhere else. I think the only place where we have it in the math library now is where it's absolutely necessary; in the memory allocation area. For consistency, that was another thing... some things were declare + defined and others were declared and defined elsewhere, within the same class. That was a bit confusing. If it wasn't detrimental to the code, I thought it was better for it to look like the rest of the codebase unless there was a good reason otherwise. For adding methods to matrix_gpu, there wasn't an objection to making the class heavier.

…

On Sun, Aug 19, 2018 at 5:39 AM seantalts ***@***.***> wrote: Also just a note for those following this thread: some discussion has continued on the proposal above here: bstatcomp#7 <bstatcomp#7> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#966 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAZ_F9IzttQ3rhwBacP2WihZ_IeQo92vks5uSTI3gaJpZM4VvFmt> .

[seantalts]

Just waiting for tests to pass now.

[seantalts]

hmmm: http://d1m1s1b1.stat.columbia.edu:8080/blue/organizations/jenkins/Math%20Pipeline/detail/PR-966/23/pipeline

[seantalts]

Okay think I fixed it that one by adding the GPU #ifdefs

[rok-cesnovar]

Thanks @seantalts for all the help and effort in helping us improve this! Really appreciated.

[seantalts]

The GPU tests no longer run on a PR and they broke on develop I think because we all forgot about a GPU test that isn't in the GPU folder:
test/unit/math/prim/mat/fun/opencl_copy_test.cpp
Should we move that into the gpu test folder? or
The test compiles again when #include <stan/math/gpu/copy.hpp> is added.

[rok-cesnovar]

I would move it. Can we do it in #1001 ?