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

Add a hicsparse backend to linalg sparse #246

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

l90lpa
Copy link
Contributor

@l90lpa l90lpa commented Nov 26, 2024

This PR adds a hicsparse backend to the sparse matrix multiplication routines of linalg sparse.

This PR depends on #247 and the branch of this PR is currently based on the branch of #247.

Notes:

@l90lpa l90lpa force-pushed the feature/add-hicsparse-backend-to-linalg-sparse branch from 50d67ca to 5f88ba1 Compare December 6, 2024 23:38
@l90lpa
Copy link
Contributor Author

l90lpa commented Dec 6, 2024

I've rebased this PR as its dependency has now changed from #241 to #247 (see comment #247 (comment)).

@wdeconinck wdeconinck force-pushed the feature/add-hicsparse-backend-to-linalg-sparse branch from 5f88ba1 to 987cdc5 Compare December 9, 2024 12:17
Copy link
Member

@wdeconinck wdeconinck left a comment

Choose a reason for hiding this comment

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

This looks nearly perfect. Very nice work, thanks! 🙏
I rebased it on develop which now contains the dependent #247 .
What's missing still is support for having the SourceValue and TargetValue be of a different precision than the MatrixValue.
This is e.g. the case with the OpenMP backend. The matrix is typically double precision (eckit::linalg::Scalar), whereas the SourceValue and TargetValue come from the atlas Fields which can be single or double.
Check e.g. this hicsparse_matrix_multiply in https://github.com/ecmwf/atlas/blob/develop/src/tests/linalg/test_linalg_sparse_matrix_gpu.cc#L32 showing the different precisions.
Note that alpha and beta arguments need to be of a chosen "ComputeType". In above linked routine I chose ComputeType to match the MatrixValue type, but in principle that is another user-choice that could be made...

@wdeconinck
Copy link
Member

Possibly this hicsparse backend only works for fields which are contiguous in memory, e.g. not separate slices of a larger array with stride != 1.
We may need to abort if that is the case. It would be good to test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants