Skip to content
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

[oneMKL][spblas] update the spec for several spblas routines #491

Merged

Conversation

baeseung-intel
Copy link
Contributor

Goal:
As oneapi::mkl::sparse domain has been evolving, we find out that we need to update the oneapi spec for some routines in oneMKL/spblas domain.

Details:
In this PR, I've updated several things:

  • fixed some minor typos, which is added const keyword incorrectly to the output parameter.
  • added asynchronous execution support for set_csr_data and release_matrix_handle routines.
  • for optimize_{gemv, trmv, trsv} routines, combined the separate APIs for sycl::bufferandUSM` containers into one common API.

void release_matrix_handle (oneapi::mkl::sparse::matrix_handle_t handle,
const std::vector<sycl::event> &dependencies = {});
sycl::event release_matrix_handle (sycl::queue &queue,
oneapi::mkl::sparse::matrix_handle_t *handle,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets name this p_handle instead of just handle :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be good to do in initializematrixhandle.rst as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. :)

Comment on lines 31 to 47

.. container:: section

.. rubric:: Input parameter

p_handle
The address of the sparse::matrix_handle_t ``p_handle`` object to be initialized.
This initialization routine must only be called on an uninitialized matrix_handle_t object.


.. container:: section

.. rubric:: Output parameters

p_handle
On return, the address is updated to point to a newly allocated and initialized matrix_handle_t object
that can be filled and used to perform sparse BLAS operations.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to have Input parameter and Output parameters sections as well in init_matrix_handle routine, so I added them as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome! :) I think it did a similar thing once upon a time to our oneMKL documentation as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Section name is just changed as Input parameters. :)

@@ -101,7 +28,7 @@ optimize_trmv (USM version)
oneapi::mkl::transpose transpose_val,
oneapi::mkl::diag diag_val,
oneapi::mkl::sparse::matrix_handle_t handle,
std::vector<sycl::event> &dependencies);
std::vector<sycl::event> &dependencies = {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might need a const out front as well. right ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it is a const reference

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. I just made it with default value. I'll check and update as necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added const keyword for the dependencies list in those optimize_xyz routines.

Copy link
Contributor

@spencerpatty spencerpatty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these changes look good. Approved. They are necessary to make Sparse BLAS more functional, prior to implementation in the oneMKL Interfaces library.

@spencerpatty spencerpatty merged commit 84ec400 into uxlfoundation:main Sep 5, 2023
3 checks passed
aepanchi pushed a commit to aepanchi/oneAPI-spec that referenced this pull request Oct 31, 2023
…dation#491)

* update spec for oneapi::mkl::sparse::release_matrix_handle() routine.

* update spec for oneapi::mkl::sparse::set_csr_data() routine.

* fix minor typo in the spec of the oneapi::mkl::sparse::gemm routine.

* fix minor typo in the spec of the oneapi::mkl::sparse::gemv routine.

* update spec for oneapi::mkl::sparse::optimize_gemv() routine.

* update spec for oneapi::mkl::sparse::optimize_trmv() routine.

* update spec for oneapi::mkl::sparse::optimize_trsv() routine.

* rename 'handle' as 'p_handle' for init_matrix_handle and release_matrix_handle routines.

* add missing 'const' keyword for dependencies list.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants