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

[BLAS:: SYCL-BLAS backend] Enable sycl blas routines #277

Merged

Conversation

pgorlani
Copy link
Contributor

@pgorlani pgorlani commented Feb 7, 2023

Description

This PR enables the following routines in the SYCL-BLAS backend: sbmv, tbmv, spmv, tpmv, tbsv, trsv and rotmg.

This patch depends on #262 and #264.

Checklist

All Submissions

  • [*] Do all unit tests pass locally? Logs: test_results.txt
    Failing ones are due to requiring fp64 support: Intel(R) UHD Graphics 770 [0x4680] is not supported.

  • [*] Have you formatted the code using clang-format?

New features

  • [*] Have you provided motivation for adding a new feature?
  • [*] Have you added relevant tests?

@pgorlani pgorlani changed the title Enable sycl blas routines [BLAS] Enable sycl blas routines Feb 20, 2023
@mmeterel mmeterel changed the title [BLAS] Enable sycl blas routines [BLAS:: SYCL-BLAS backend] Enable sycl blas routines Apr 12, 2023
@pgorlani pgorlani force-pushed the enable-sycl-blas-routines branch 2 times, most recently from a20f662 to 925c898 Compare April 25, 2023 08:50
@pgorlani pgorlani force-pushed the enable-sycl-blas-routines branch from 4d39eab to d334cd8 Compare May 12, 2023 13:25
@pgorlani pgorlani marked this pull request as draft May 12, 2023 14:21
@pgorlani pgorlani force-pushed the enable-sycl-blas-routines branch from d334cd8 to 79bfe76 Compare June 1, 2023 13:05
@pgorlani pgorlani marked this pull request as ready for review June 1, 2023 13:05
Copy link
Contributor

@mkrainiuk mkrainiuk 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, thank you!

@muhammad-tanvir-1211
Copy link
Contributor

muhammad-tanvir-1211 commented Jun 15, 2023

Thanks @mkrainiuk.
@andrewtbarker could you please take a look at this PR here and let us know if we need to make any changes?

Copy link
Contributor

@andrewtbarker andrewtbarker left a comment

Choose a reason for hiding this comment

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

Sorry for the late review. This looks fine to me, can you test the newly supported APIs on nvidia hardware to make sure that configuration is working before we merge?

@pgorlani
Copy link
Contributor Author

Sorry for the late review. This looks fine to me, can you test the newly supported APIs on nvidia hardware to make sure that configuration is working before we merge?

No problem! Here are the test logs for the NVIDIA gpu, NVIDIA_GPU_test_results.txt.

Thanks for your reviews, @mkrainiuk and @andrewtbarker.

We are planning to enable more SYCL-BLAS routines for the future. This will involve other PRs very similar to this, i.e., each changed line will add a routine. In this regard, we are wondering what could be an ideal PR size that could ease your reviewing tasks.

Do you prefer fewer PRs containing more routines, or more PRs containing fewer routines?

@andrewtbarker
Copy link
Contributor

Do you prefer fewer PRs containing more routines, or more PRs containing fewer routines?

Speaking only for myself, for a PR like this I think it makes sense to include many routines in few PRs. I like to run the tests locally for confirmation and that will be easier to do this way.

If you were changing logic / algorithms it would be a different story, then smaller PRs would likely be better.

@mkrainiuk
Copy link
Contributor

Do you prefer fewer PRs containing more routines, or more PRs containing fewer routines?

Speaking only for myself, for a PR like this I think it makes sense to include many routines in few PRs. I like to run the tests locally for confirmation and that will be easier to do this way.

If you were changing logic / algorithms it would be a different story, then smaller PRs would likely be better.

Same here, thanks!

@pgorlani
Copy link
Contributor Author

Do you prefer fewer PRs containing more routines, or more PRs containing fewer routines?

Speaking only for myself, for a PR like this I think it makes sense to include many routines in few PRs. I like to run the tests locally for confirmation and that will be easier to do this way.
If you were changing logic / algorithms it would be a different story, then smaller PRs would likely be better.

Same here, thanks!

I was thinking the same. Thanks for your feedbacks, @mkrainiuk and @andrewtbarker.

@muhammad-tanvir-1211 muhammad-tanvir-1211 merged commit efa1165 into uxlfoundation:develop Jun 19, 2023
normallytangent pushed a commit to normallytangent/oneMKL that referenced this pull request Aug 6, 2024
Added rotmg, sbmv, tbmv, spmv, tbsv, trsv and tpmv BLAS operators.
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.

4 participants