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

[SPARSE] Add support for rocSPARSE backend #544

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

Rbiessy
Copy link
Contributor

@Rbiessy Rbiessy commented Jul 24, 2024

Description

Add support for the rocSPARSE backend.

Depends on #527 and #532.

Rendered docs: docs.zip

Checklist

All Submissions

@Rbiessy Rbiessy requested a review from a team July 24, 2024 13:57
@Rbiessy Rbiessy changed the title [SPARSE] Add support for rocsparse backend [SPARSE] Add support for rocSPARSE backend Jul 24, 2024
@Rbiessy
Copy link
Contributor Author

Rbiessy commented Jul 30, 2024

Tests log on W6800: amd_w6800_log.txt

@gajanan-choudhary gajanan-choudhary added backend A request to enable new implementation behind API Sparse BLAS domain Sparse BLAS domain issue/request feature A request to add a new feature labels Sep 12, 2024
@gajanan-choudhary
Copy link
Contributor

gajanan-choudhary commented Oct 29, 2024

@Rbiessy, now that #527 is merged in, could you please rebase this branch against main, resolve conflicts, and force-push when you can so that I can start reviewing this PR?

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Oct 29, 2024

Yes, this is in progress! I expect I will need a few days at least. I'm aiming to merge the PR by the end of the year.

@Rbiessy
Copy link
Contributor Author

Rbiessy commented Nov 1, 2024

@gajanan-choudhary I have updated the PR with recent changes from cuSPARSE. Note that it moved almost all the content from cusparse_task.hpp to common_launch_task.hpp with almost no changes.
New test logs:
log_mi210.txt
log_a100.txt
log_pvc.txt

@Rbiessy Rbiessy mentioned this pull request Nov 5, 2024
2 tasks
<td align="center">AMD GPU</td>
</tr>
<tr>
<td align="center"><a href="https://github.com/ROCmSoftwarePlatform/rocSPARSE"> AMD rocSPARSE</a></td>
Copy link
Contributor

@gajanan-choudhary gajanan-choudhary Nov 24, 2024

Choose a reason for hiding this comment

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

Please also add rocSPARSE version info in the table later in this README file in the table in the Product and Version Information section

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that, done in dc22051

Comment on lines +90 to +93
- The same sparse matrix handle cannot be reused for multiple operations
``spmm``, ``spmv``, or ``spsv``. Doing so will throw a
``oneapi::mkl::unimplemented`` exception. See `#332
<https://github.com/ROCm/rocSPARSE/issues/332>`_.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, this is quite severe, but seems to be a legitimate issue on rocSPARSE side right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

If and when they fix this issue, though, will it be easy for us to make changes (with a version check of course) that correctly performs the operations rather than throwing an unimplemented exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's easy to fix on oneMath side. The issue is also referenced in this comment: https://github.com/oneapi-src/oneMKL/pull/544/files#diff-3b8c1c2c71abd54f8f90f43415c2f17b2a7fdb81c2b882c210f3cba56b4679adR63
One would just need to remove the used member, its 2 usages below as well as the mark_used method.

@@ -22,6 +22,7 @@
#include "cusparse_error.hpp"
#include "cusparse_helper.hpp"
#include "cusparse_handles.hpp"
#include "cusparse_scope_handle.hpp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is a cuSPARSE change coming in here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it related to the common_launch_task.hpp to keep common code between cuSPARSE and rocSPARSE backends?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just as you said, cusparse_task.hpp used to include cusparse_scope_handle.hpp but it's not needed anymore (with the already merged changes to use the ScopeHandle object less often) and not possible anymore with the introduction of the more generic common_launch_task.hpp.

#include "rocsparse_error.hpp"
#include "sparse_blas/backends/common_launch_task.hpp"

namespace oneapi::mkl::sparse::rocsparse::detail {
Copy link
Contributor

@gajanan-choudhary gajanan-choudhary Nov 24, 2024

Choose a reason for hiding this comment

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

Other than cusparse -> rocsparse, this file, rocsparse/rocsparse_task.hpp appears to be nearly identical to cusparse/cusparse_task.hpp. Is there anything more than that in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they are almost identical. I'm not sure I understand, why do you ask?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend A request to enable new implementation behind API feature A request to add a new feature Sparse BLAS domain Sparse BLAS domain issue/request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants