Adding cuBLAS backend to oneMKL. #2

mehdi-goli · 2020-04-14T20:57:54Z

This PR adds cuBLAS backend to OneMKL.

Requirements
To compile the cuBLAS backend PR 1332 is required.

Known issue
The test suit should run via ctest. When the tests run stand-alone(e.g ./bin/test_main_ct or ./bin/test_main_rt) , it can lead to segmentation fault due to issue 1520.

src/blas/backends/cublas/cublas_scope_handle.cpp

vmalia

In general, all new files lack a newline at the end of file. Can we ensure that an empty newline exists so that we comply with POSIX standard for file and line endings?
I am unable to find an option that enables this check in clang-format.

cmake/FindcuBLAS.cmake

README.md

include/onemkl/blas/detail/cublas/blas_ct.hpp

src/blas/backends/cublas/CMakeLists.txt

mmeterel · 2020-04-17T06:01:01Z

src/blas/backends/cublas/cublas_level1.cpp

+            // By default the pointer mode is the CUBLAS_POINTER_MODE_HOST
+            // when the data is on buffer, it must be set to
+            // CUBLAS_POINTER_MODE_DEVICE mode otherwise it causes the segmentation
+            // fault. When it is said to device it is users responsibility to


said? is this a typo?

mmeterel · 2020-04-17T06:01:48Z

src/blas/backends/cublas/cublas_level1.cpp

+    // mimic iamax
+    // we are converting the result to be the int and then we convert it back to
+    // the actual data on the host
+    // FIXME:: this change may cause failiour as the result of integer overflow


FIXME? Is this supposed to be fixed already? Also, there are typos in the comments.

Yes, the comment was outdated, I have updated the comments.

mmeterel · 2020-04-17T06:02:45Z

src/blas/backends/cublas/cublas_level1.cpp

+    // cuda does not support int64_t as return type for the data. So we need to
+    // mimic iamax we are converting the result to be the int and then we convert
+    // it back to the actual data on the host
+    // FIXME:: this change may cause failure as the result of integer overflow


FIXME? Is this supposed to be fixed already?

Yes, the comment was outdated, I have updated the comments.

mmeterel · 2020-04-17T06:05:05Z

src/blas/backends/cublas/cublas_level1.cpp

+    // converting float* to double * is very constly as sycl reinterpret does not
+    // support conversion from two types which is not the same size. So in order,
+    // to avoid loosing performance we are converting the result to be the float
+    // FIXME:: this change may cause failiour as the result precision reduces.


FIXME? Is this supposed to be fixed already?

Yes, the comment was outdated, I have updated the comments.

mmeterel · 2020-04-17T06:05:52Z

src/blas/backends/cublas/cublas_level1.cpp

+            CUBLAS_ERROR_FUNC(cublasSdot, err, handle, n, x_, incx, y_, incy, float_res_);
+        });
+    });
+    /// FIXME::This is a temporary solution, this can result it precision issue.


FIXME? Is this supposed to be fixed already? result in: typo

Yes, the comment was outdated, I have updated the comments.

mmeterel · 2020-04-17T06:08:44Z

src/blas/backends/cublas/cublas_level3.cpp

+
+GEMM_LAUNCHER(float, cublasSgemm)
+GEMM_LAUNCHER(double, cublasDgemm)
+// GEMM_LAUNCHER(std::complex<float>, cublasCgemm3m) from sm5 onward can improve


What does "from sm5 onward" mean?

It is removed since the function mentioned in the comment was part of BLAS extension and not related to this PR.

mmeterel · 2020-04-17T16:32:55Z

src/blas/backends/cublas/cublas_level1.cpp

+    // converting float* to double * is very costly operation as sycl reinterpret
+    // does not support conversion from two types which is not the same size.
+    // So in order, to avoid loosing performance we are converting the result to be
+    // the float this change may cause failure as the result precision reduces.


the float. This chance...

mmeterel · 2020-04-17T16:33:27Z

src/blas/backends/cublas/cublas_level1.cpp

+    // does not support conversion from two types which is not the same size.
+    // So in order, to avoid loosing performance we are converting the result to be
+    // the float this change may cause failure as the result precision reduces.
+    // Alternatively we need to a sycl kernel to elementwise copy the


we need to write? a sycl kernel

mmeterel · 2020-04-17T16:34:03Z

src/blas/backends/cublas/cublas_level1.cpp

+        });
+    });
+    /// Since cuBLAS does not have sdot support, we had to do the operation in float and
+    // convert it back into double this can result in precision issue.


double. This can...

mmeterel · 2020-04-17T16:34:41Z

src/blas/backends/cublas/cublas_level1.cpp

+    using cuDataType = typename CudaEquivalentType<T>::Type;
+    overflow_check(n, incx);
+    // cuBLAS does not support int64_t as return type for the data. So we need to
+    // mimic iamax we are converting the result to be the int and then we convert


mimic iamax. We are converting...

mmeterel · 2020-04-17T16:35:04Z

src/blas/backends/cublas/cublas_level1.cpp

+    // mimic iamax we are converting the result to be the int and then we convert
+    // it back to the actual data on the host.
+    // This change may cause failure as the result of integer overflow
+    // based on the size. Alternatively either we need to write two a sycl kernel


write two a ?

mmeterel · 2020-04-17T16:35:31Z

src/blas/backends/cublas/cublas_level1.cpp

+                  const int64_t incx, cl::sycl::buffer<int64_t, 1> &result) {
+    using cuDataType = typename CudaEquivalentType<T>::Type;
+    overflow_check(n, incx);
+    // cuda does not support int64_t as return type for the data. So we need to


cuda -> cuBLAS

mmeterel · 2020-04-17T16:36:20Z

src/blas/backends/cublas/cublas_level1.cpp

+            int64_t incx, cl::sycl::buffer<float, 1> &y, int64_t incy,
+            cl::sycl::buffer<float, 1> &result) {
+    overflow_check(n, incx, incy);
+    // cuda does not support sdot so we need to mimic sdot


cuda -> cuBLAS?

mmeterel · 2020-04-17T16:36:51Z

src/blas/backends/cublas/cublas_level1.cpp

+    // mimic iamin we are converting the result to be the int and then we convert
+    // it back to the actual data on the host.
+    // This change may cause failure as the result of integer overflow
+    // based on the size. Alternatively, either we need to write two a sycl kernel


write two a ?

mmeterel · 2020-04-17T16:42:17Z

src/blas/backends/cublas/cublas_level1.cpp

+            cublasSetPointerMode(handle, CUBLAS_POINTER_MODE_DEVICE);
+            auto x_       = sc.get_mem<cuDataType *>(ih, x_acc);
+            auto int_res_ = sc.get_mem<int *>(ih, int_res_acc);
+            cublasStatus_t err;


support for negative incx?

iamin is similar to iamax

mmeterel · 2020-04-17T16:46:34Z

src/blas/backends/cublas/cublas_level1.cpp

+            auto x_       = sc.get_mem<cuDataType *>(ih, x_acc);
+            auto int_res_ = sc.get_mem<int *>(ih, int_res_acc);
+            cublasStatus_t err;
+            // IAMAX does not support negative incx


if that is the case, why incx is passed without std::abs()?

I think the wording did not convey the meaning. For iamax when the incx is negative cuBLAS returns 0 and does not execute the Kernel. This behavior is similar to that of reference NetlibBLAS. Because of that, we don't see the difference between the reference and cuBLAS. I think Intel's implementation is the same. So the incx should not have abs around it because by adding abs, it converts the incx to positive and the result would be different. So I have changed the wording.

vmalia · 2020-04-20T15:35:44Z

Can we have an empty new line in every file? Many files are missing those. Once those changes are in, I can approve my part of the review.

mehdi-goli · 2020-04-20T18:04:28Z

Can we have an empty new line in every file? Many files are missing those. Once those changes are in, I can approve my part of the review.

We tried to match the style of other files in oneMKL as there is no new empty line on any other files. However, I have manually added a new empty line at the end of the files that we have modified.

vmalia

Looks good, thank you!

README.md

vmalia · 2020-04-21T00:10:22Z

@mehdi-goli I apologize for my mistake and the inconvenience it may have caused you. Looks like our coding guidelines do not specify any newline requirements at the end of file, nor does clang-format support having them. You can either revert these changes before merge or we can do it on our side after merge. Which of these do you prefer?

mehdi-goli · 2020-04-21T03:53:20Z

@mehdi-goli I apologize for my mistake and the inconvenience it may have caused you. Looks like our coding guidelines do not specify any newline requirements at the end of file, nor does clang-format support having them. You can either revert these changes before merge or we can do it on our side after merge. Which of these do you prefer?

No problem. I have reverted them.

mkrainiuk requested changes Apr 15, 2020

View reviewed changes

src/blas/backends/cublas/cublas_scope_handle.cpp Show resolved Hide resolved

vmalia requested changes Apr 15, 2020

View reviewed changes

cmake/FindcuBLAS.cmake Outdated Show resolved Hide resolved

README.md Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

mkrainiuk reviewed Apr 16, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

include/onemkl/blas/detail/cublas/blas_ct.hpp Show resolved Hide resolved

src/blas/backends/cublas/CMakeLists.txt Outdated Show resolved Hide resolved

mkrainiuk approved these changes Apr 16, 2020

View reviewed changes

mmeterel reviewed Apr 17, 2020

View reviewed changes

nyalloc mentioned this pull request Apr 17, 2020

Add __SYCL_EXPORT to declaration of contextSetExtendedDeleter intel/llvm#1531

Merged

mmeterel reviewed Apr 17, 2020

View reviewed changes

mehdi-goli requested a review from vmalia April 20, 2020 08:43

vmalia approved these changes Apr 20, 2020

View reviewed changes

mkrainiuk reviewed Apr 20, 2020

View reviewed changes

README.md Show resolved Hide resolved

Adding cuBLAS backend to oneMKL.

a7e8fa2

jasukhar merged commit 7514726 into uxlfoundation:master Apr 21, 2020

trosenqu mentioned this pull request Sep 28, 2021

Initial pages site content #130

Merged

Adding cuBLAS backend to oneMKL. #2

Adding cuBLAS backend to oneMKL. #2

Conversation

mehdi-goli commented Apr 14, 2020

vmalia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mehdi-goli Apr 17, 2020 • edited Loading

Choose a reason for hiding this comment

vmalia commented Apr 20, 2020 • edited Loading

mehdi-goli commented Apr 20, 2020

vmalia left a comment

Choose a reason for hiding this comment

vmalia commented Apr 21, 2020

mehdi-goli commented Apr 21, 2020

mehdi-goli Apr 17, 2020 •

edited

Loading

vmalia commented Apr 20, 2020 •

edited

Loading