Skip to content

Comments

Add hipSYCL curand support#226

Merged
mkrainiuk merged 9 commits intouxlfoundation:developfrom
nilsfriess:hipsycl-curand-support
Oct 19, 2022
Merged

Add hipSYCL curand support#226
mkrainiuk merged 9 commits intouxlfoundation:developfrom
nilsfriess:hipsycl-curand-support

Conversation

@nilsfriess
Copy link
Contributor

Description

This PR adds support to use the cuRAND backend with hipSYCL.
The approach is similar though simpler than #144: Since hipSYCL does not support host_task but instead implements the extension hipSYCL_enqueue_custom_operation, a wrapper function onemkl_curand_host_task is introduced that calls either host_task or hipSYCL_enqueue_custom_operation depending on which compiler is used.

Further, calls to curandSetStream before calling the cuRAND generate-functions were added. This is important since previously, the call to wait_and_throw after submitting the CUDA calls might not wait for the cuRAND random number generation to finish if it was started on a different stream. In fact, increasing the amount of random numbers generated in the unit tests (and thereby increasing the time spent generating numbers) made some tests fail (both when using hipSYCL and dpc++), since the result might be read before random number generation is finished. Now all tests pass also when increasing the number of random samples to be generated.

Test logs

The tests were executed on Ubuntu 20.04 using CUDA 11.6.
curand_dpcpp_test.log
curand_hipsycl_test.log

Checklist

  • Do all unit tests pass locally? Attach a log.
  • Have you formatted the code using clang-format?

Copy link
Contributor

@aelizaro aelizaro left a comment

Choose a reason for hiding this comment

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

Hi @nilsfriess, thank you for the PR, looks good to me! @mkrainiuk, could you, please, also take a look?

@aelizaro aelizaro requested a review from mkrainiuk September 30, 2022 10:54
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, it also looks good to me. Does it make sense to update also our documentation with RNG supported compilers?

@nilsfriess
Copy link
Contributor Author

I think there are multiple places where the documentation is not quite up to date with regard to hipSYCL support (e.g. here and here it is not mentioned that hipSYCL can also be used for the BLAS backend with CUDA).
Should I make an extra PR for the documentation updates or just include it in this one?

@mkrainiuk
Copy link
Contributor

I think there are multiple places where the documentation is not quite up to date with regard to hipSYCL support (e.g. here and here it is not mentioned that hipSYCL can also be used for the BLAS backend with CUDA). Should I make an extra PR for the documentation updates or just include it in this one?

Good catch, we need to update them too. I'm fine with both options, if you prefer extra PR we can merge this PR as is.

@nilsfriess nilsfriess force-pushed the hipsycl-curand-support branch from 2a17e78 to 2f79496 Compare October 19, 2022 08:47
@nilsfriess nilsfriess force-pushed the hipsycl-curand-support branch from 2f79496 to 0a7db36 Compare October 19, 2022 08:48
@nilsfriess
Copy link
Contributor Author

There weren't too many places where the documentation was not up to date, so I just added the changes to this PR.

@mkrainiuk
Copy link
Contributor

There weren't too many places where the documentation was not up to date, so I just added the changes to this PR.

Thank you! Documentation looks good to me. Please let me know if there anything else you want to add or we can merge it now.

@nilsfriess
Copy link
Contributor Author

Thanks, you can merge it :)

@mkrainiuk mkrainiuk merged commit ce6226f into uxlfoundation:develop Oct 19, 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