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

Added const qualifier to USM API #443

Merged
merged 8 commits into from
Aug 21, 2023
Merged

Conversation

muhammad-tanvir-1211
Copy link
Collaborator

This PR adds the const qualifier to input containers for the USM api as required by the oneMKL specification.

@muhammad-tanvir-1211 muhammad-tanvir-1211 added the stacked PR is stacked on top of another label Jun 22, 2023
@muhammad-tanvir-1211 muhammad-tanvir-1211 marked this pull request as draft June 22, 2023 13:13
@aacostadiaz aacostadiaz marked this pull request as ready for review July 20, 2023 08:53
include/blas_meta.h Outdated Show resolved Hide resolved
src/interface/blas3/gemm_launcher.cpp.in Outdated Show resolved Hide resolved
src/interface/blas1/asum.cpp.in Outdated Show resolved Hide resolved
src/interface/blas1/axpy.cpp.in Show resolved Hide resolved
src/interface/blas2/gemv.cpp.in Outdated Show resolved Hide resolved
include/operations/blas2_trees.h Outdated Show resolved Hide resolved
@@ -79,8 +79,10 @@ typename sb_handle_t::event_t _gemv_impl(
const auto x_vector_size = is_transposed ? _M : _N;
const auto y_vector_size = is_transposed ? _N : _M;

auto mA = make_matrix_view<col_major>(_mA, _M, _N, _lda);
auto vx = make_vector_view(_vx, _incx, x_vector_size);
typename MatrixViewType<container_t0, index_t, col_major>::type mA =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems all the changes in this file is unnecessary

Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes to MatrixViewType<..> and VectorViewType<..> are necessary because we cannot replace them with const auto. The problem is that, in the case of buffer, VectorView/MatrixView cannot be const because of the access_displacement.

@@ -188,7 +191,9 @@ template <typename sb_handle_t, typename container_0_t, typename container_1_t,
typename sb_handle_t::event_t _asum(
sb_handle_t &sb_handle, index_t _N, container_0_t _vx, increment_t _incx,
container_1_t _rs, const typename sb_handle_t::event_t &_dependencies) {
auto vx = make_vector_view(_vx, _incx, _N);
using element_t = typename ValueType<container_0_t>::type;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems unnecessary

@@ -86,7 +87,9 @@ typename sb_handle_t::event_t _copy(
sb_handle_t &sb_handle, index_t _N, container_0_t _vx, increment_t _incx,
container_1_t _vy, increment_t _incy,
const typename sb_handle_t::event_t &_dependencies) {
auto vx = make_vector_view(_vx, _incx, _N);
using element_t = typename ValueType<container_0_t>::type;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems unnecessary

@@ -306,18 +306,19 @@ make_gemm(input_t buffer_a, input_t buffer_b, output_t buffer_c,
* @Note This kernel assumes the column-major matrices
* @Note This kernel uses fixed size blocks of 16, but this can be changed
*/
template <bool UnitDiag, bool Upper, int BlockSize, typename matrix_t>
template <bool UnitDiag, bool Upper, int BlockSize, typename lhs_t,
Copy link
Collaborator

@mehdi-goli mehdi-goli Aug 3, 2023

Choose a reason for hiding this comment

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

this is the name changing. I don't think this changes related to const qualifier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This name change was required to differentiate between the input and output container types. The input in this case can be const and the output cannot be const, thus we needed separate template parameters to represent the two different types.

Copy link
Collaborator

@mehdi-goli mehdi-goli left a comment

Choose a reason for hiding this comment

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

I have added minor comment overall looks OK. Thanks for the clean up

@aacostadiaz aacostadiaz force-pushed the usm_experimental branch 3 times, most recently from 07f1e84 to 37be9e4 Compare August 11, 2023 08:36
aacostadiaz and others added 2 commits August 11, 2023 13:42
* Fix negative increment

* Fix build with computecpp

* Update dpcpp

* Revert dpcpp version

* Fix sync call

* Add a pointer to the first element of the vector

* Fixes issue with dependencies in nrm2 (#451)

* Fix nrm2

* Remove cout

* Fixes issue with dependencies in TBMV (#454)

This PR fixes an issue with dependencies in TBMV (similar to the fix in #451)
@aacostadiaz aacostadiaz force-pushed the fix_const_usm branch 2 times, most recently from 0e82703 to 7d7cccd Compare August 11, 2023 16:02
include/interface/blas2_interface.h Outdated Show resolved Hide resolved
@@ -306,18 +306,19 @@ make_gemm(input_t buffer_a, input_t buffer_b, output_t buffer_c,
* @Note This kernel assumes the column-major matrices
* @Note This kernel uses fixed size blocks of 16, but this can be changed
*/
template <bool UnitDiag, bool Upper, int BlockSize, typename matrix_t>
template <bool UnitDiag, bool Upper, int BlockSize, typename lhs_t,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This name change was required to differentiate between the input and output container types. The input in this case can be const and the output cannot be const, thus we needed separate template parameters to represent the two different types.

cmake/CmakeFunctionHelper.cmake Outdated Show resolved Hide resolved
Copy link
Collaborator

@OuadiElfarouki OuadiElfarouki left a comment

Choose a reason for hiding this comment

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

Looks good to me. Tested it on some devices, getting compile errors with Nvidia SM>=80 though.
/src/operations/blas3/gemm_local_joint_matrix.hpp:295:18: error: no matching conversion for functional-style cast from 'container_t' (aka 'const float *') to 'multi_ptr_' (aka 'multi_ptr<float, address_t::global_space>')
All the rest seems fine, error doesn't occur when disabling joint-matrix GEMM.

@aacostadiaz aacostadiaz merged commit 8babe16 into usm_experimental Aug 21, 2023
5 checks passed
aacostadiaz added a commit that referenced this pull request Aug 22, 2023
This PR adds the const qualifier to input containers for the USM api as required by the oneMKL specification.
---------

Co-authored-by: Muhammad Tanvir <[email protected]>
Co-authored-by: Alejandro Acosta <[email protected]>
aacostadiaz added a commit that referenced this pull request Aug 22, 2023
This PR adds the const qualifier to input containers for the USM api as required by the oneMKL specification.
---------

Co-authored-by: Muhammad Tanvir <[email protected]>
Co-authored-by: Alejandro Acosta <[email protected]>
@muhammad-tanvir-1211 muhammad-tanvir-1211 deleted the fix_const_usm branch October 23, 2023 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stacked PR is stacked on top of another
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants