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

Enable using oneMKL with hipSYCL #99

Closed
sbalint98 opened this issue Jun 9, 2021 · 6 comments
Closed

Enable using oneMKL with hipSYCL #99

sbalint98 opened this issue Jun 9, 2021 · 6 comments
Labels
RFC A proposal to add new API

Comments

@sbalint98
Copy link
Contributor

sbalint98 commented Jun 9, 2021

Summary

With a set of Pull requests, we would like to upstream changes to enable the use of oneMKL BLAS with hipSYCL. The changes and added features encompass the following:

Problem statement

Our intention is to make oneMKL more available for the general SYCL community. These changes will allow easier integration with other SYCL implementation and enable adding rocm libraries to oneMKL.

In case the PRs from #100, #101, #102, #104, #105 are merged, adding the actual hipSYCL support could look like this: https://github.com/sbalint98/oneMKL/tree/ustream-hipsycl-specific-changes .

when 100-103 is merged into the current develop e8e3dab, all tests pass locally: int_test_oneapi.log

@mmeterel
Copy link
Contributor

mmeterel commented Jun 9, 2021

@sbalint98
Thanks for all the PRs. I have a general question for my better understanding.
Could you please provide more information on what capabilities these PRs are adding? I guess I am not too familiar with hipSYCL, so it would be great if you can give some background. Do you have any future plans after these PRs? For example, will you be adding ROCm backend in the future? Are these PRs enabling that path?

@sbalint98
Copy link
Contributor Author

In general, the currently open PRs make small changes to make adding hipSYCL more elegant. After these (100-103) a last PR can be opend, that will add the actual hipSYCL specific details.

In order to enable adding hipSYCL the following changes are necessary, appart from the ones in #100:
#101 calls the add_sycl_to_target cmake command on all relevant targets when compiling for hipSYCL. This is going to be needed by other SYCL implementations as well.

#102 Is required since hipSYCL does not support cl::sycl::half yet. It adds a CMake variable that can be toggled to disable all BLAS functions that operate on cl::sycl::half datatype.

Furthermore, hipSYCL does not implement interop_task. We have hipsycl_enqueue_custom_operation instead, which is a hipSYCL extension. To make replacing interop_task when using hipSYCL easy, in #103 a small restructuring is carried out so that we can hide both of them behind a common interface, and we can use ifdefs to switch between them. (see here)

After these PRs, it would be logical to continue with adding support for rocm. A preliminary implementation is already available here. However, it will need more testing until a PR could be opened here. As far as I understand, since dpc++ does not support ROCm devices currently, adding support for a SYCL implementation, that does, is the only way to enable it in a way that it can be actually used.

I ping @illuhad, he is able to answer questions more in depth, if necessary.

@vrpascuzzi
Copy link
Contributor

@sbalint98 A similar solution to #103 will be needed also for cuRAND. (I needed to use hipsycl_enqueue_custom_operation in my own work when requiring {hip,roc}RAND.)

@illuhad
Copy link

illuhad commented Jun 9, 2021

For example, will you be adding ROCm backend in the future? Are these PRs enabling that path?

In short: Yes, this is exactly what this is about :) We are currently working on porting oneAPI libraries to hipSYCL/AMD hardware, including oneMKL.

Apart from the benefit of additional hardware support, adding support for new SYCL implementations can also be beneficial to make the code more stable. For example, we found the bug from PR #96 by testing with hipSYCL (apparently, it does not show up with DPC++ even though it's clearly a bug).

One thing I see that might need discussion is how you would like to handle CI. Would it be possible to add hipSYCL to your CI infrastructure? Or would it be more appropriate to let it run on github actions infrastructure - at least for testing on CPU?

EDIT: Thanks @sbalint98 for all this work :-)

@mmeterel
Copy link
Contributor

@illuhad, @sbalint98

Thank you for your answers and clarifications. I still have a few questions. Do you think it would be possible to arrange a small meeting so that we can have a more in depth discussion? If that is ok, I can setup the meeting. You can reach to me directly at mesut.meterelliyoz@intel.com to discuss meeting details.

@sbalint98 sbalint98 mentioned this issue Nov 20, 2021
3 tasks
@mmeterel
Copy link
Contributor

Closing issue. It is being tracked by separate PRs. #144 #156

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC A proposal to add new API
Projects
None yet
Development

No branches or pull requests

4 participants