-
Notifications
You must be signed in to change notification settings - Fork 158
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BLAS][portBLAS] Add bindings for half and some gemm_batch group APIs #576
Conversation
Hello @al3x-jp, I have been told you are keen to test oneMKL+portBLAS with llama.cpp. This branch should enable everything that is needed. Would you be able to try it and let us know if you run into any issue? |
@@ -45,6 +45,11 @@ foreach(domain ${TARGET_DOMAINS}) | |||
add_subdirectory(${domain}) | |||
endforeach() | |||
|
|||
if (PORTBLAS_ENABLE_HALF) | |||
# Set the variable used for C++ macro | |||
set(ENABLE_PORTBLAS_HALF ON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I was planning to update this PR once #571 is merged. It will be easier to do once I can add ONEAPI_ONEMKL_ENABLE_PORTBLAS_HALF
to the list you introduce in https://github.com/oneapi-src/oneMKL/pull/571/files#diff-148715d6ea0c0ea0a346af3f6bd610d010d490eca35ac6a9b408748f7ca9e3f4R54
@@ -695,7 +710,7 @@ sycl::event gemm_batch(sycl::queue &queue, oneapi::mkl::transpose *transa, | |||
const double **b, std::int64_t *ldb, double *beta, double **c, | |||
std::int64_t *ldc, std::int64_t group_count, std::int64_t *group_size, | |||
const std::vector<sycl::event> &dependencies) { | |||
throw unimplemented("blas", "gemm_batch", " for USM"); | |||
throw unimplemented("blas", "gemm_batch", " for USM using double"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you explain me why it doesn't work with double
? I thought that in portBLAS if it works with float
it works with double
. Did you test it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can probably be added, I'm looking this. I have found that the gemm_batch tests only use group_count=5
. I need to test this with group_count=1
.
// BUFFER | ||
void gemm(sycl::queue &queue, oneapi::mkl::transpose transa, oneapi::mkl::transpose transb, | ||
std::int64_t m, std::int64_t n, std::int64_t k, sycl::half alpha, | ||
sycl::buffer<sycl::half, 1> &a, std::int64_t lda, sycl::buffer<sycl::half, 1> &b, | ||
std::int64_t ldb, sycl::half beta, sycl::buffer<sycl::half, 1> &c, std::int64_t ldc) { | ||
#ifdef ENABLE_PORTBLAS_HALF | ||
CALL_PORTBLAS_FN(::blas::_gemm, queue, transa, transb, m, n, k, alpha, a, lda, b, ldb, beta, c, | ||
ldc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, we don't support any row major operator in portBLAS, so they are generally not enabled.
Even if it works now, unless it is necessary, I would not enable it to avoid the confusion of having only one row major operator supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are already checks that throw unimplemented when row_major
is used here: https://github.com/oneapi-src/oneMKL/blob/develop/src/blas/backends/portblas/portblas_common.hpp#L193
All the other functions have already their bindings ready for row_major
because they use this pattern: https://github.com/oneapi-src/oneMKL/blob/develop/src/blas/backends/portblas/portblas_level3_float.cpp#L54
Just portblas_level3_half.cpp
looks a bit different
Closing as I don't think there is enough interest right now. |
Description
Add missing features needed to run llama.cpp with oneMKL+portBLAS:
sycl::half
bindingsbatch_gemm
Checklist
All Submissions
Do all unit tests pass locally? Log using the CT tests and examples: test_log_portblas_example.txt test_log_portblas_ct.txt. The tests are currently very slow so not all the RT tests were run.
Have you formatted the code using clang-format?