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, cuBLAS backend, hipSYCL] Add official hipsycl cuBLAS support #166

Merged
merged 2 commits into from
Sep 16, 2022

Conversation

sbalint98
Copy link
Contributor

As discussed under PR #144 hipSYCL rocBLAS support comes with automatic support for cuBLAS as well. This PR adds this information to the README.

Furthermore, this PR differentiates the FindcuBLAS.cmake file between hipSYCL and Intel LLVM. In the case of hipSYCL, there is no need to have the OPENCL_PATH set. Moreover, the CUDA_CUDA_LIBRARY environment variable is also not strictly necessary since finding the libcuda.so is handled by hipSYCL.

onemkl-cuda-hipsycl-test.txt

@mmeterel mmeterel changed the title Add official hipsycl cuBLAS support [BLAS, cuBLAS backend, hipSYCL] Add official hipsycl cuBLAS support May 11, 2022
@mmeterel
Copy link
Contributor

@sbalint98 As you are aware, there were several changes since the time this PR is opened. Could you please rebase and check if the tests are all good? Also, I think, we can enable cuRAND backend with hipSYCL with this PR as well. What do you think?

Could you please let me know when everything is ready?

@sbalint98
Copy link
Contributor Author

Hi @mmeterel thank you for reminding me. I'll have a look and prepare this PR until Monday. I'll let you know if it is ready for review

@sbalint98 sbalint98 force-pushed the add-hipsycl-cublas branch from 7e9cd2d to 5c11023 Compare June 6, 2022 16:06
@sbalint98
Copy link
Contributor Author

Sorry for the delay. I have rebased the branch and updated the readme. All tests are passing on my sid
cublas_tests_hipsycl.txt
e

@mmeterel
Copy link
Contributor

mmeterel commented Jun 6, 2022

Sorry for the delay. I have rebased the branch and updated the readme. All tests are passing on my sid cublas_tests_hipsycl.txt e

@sbalint98 Thanks. Do you plan to add cuRAND support in this PR as well?

@sbalint98
Copy link
Contributor Author

sbalint98 commented Jun 6, 2022

@nilsfriess and @normallytangent are working on the cuRAND support. I believe for them it might be easier to open a new PR. Would this be acceptable for you?

@mmeterel
Copy link
Contributor

mmeterel commented Jun 6, 2022

@nilsfriess and @normallytangent are working on the cuRAND support. I believe for them it might be easier to open a new PR. Would this be acceptable for you?

Sure, that works if it will be easier on your side.

@mmeterel
Copy link
Contributor

@sbalint98
Thanks for the PR. I tested it locally (had to rebase hipSYCL and build it with LLVM-13) and it works smoothly. :)

@mmeterel
Copy link
Contributor

@mkrainiuk Could you please take a look at this?

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.

Thank you for the PR! Changes look good to me. I have one minor comment.

CUDA_LIBRARIES
CUDA_CUDART_LIBRARY
)
set_target_properties(ONEMKL::cuBLAS::cuBLAS PROPERTIES
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reasons we cannot use one set_target_properties? I see the only difference: OPENCL_INCLUDE_DIR that can be added with inline check, something like "$<$BOOL:${USE_ADD_SYCL_TO_TARGET_INTEGRATION}: ${OPENCL_INCLUDE_DIR}>"

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure...but i also see differences between CUDA_CUDART_LIBRARY vs CUDA_CUDA_LIBRARY

@mmeterel mmeterel merged commit 8bf615f into uxlfoundation:develop Sep 16, 2022
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.

3 participants