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

Use of sycl::half, with CL/sycl.hpp SYCL header included #149

Closed
sbalint98 opened this issue Dec 3, 2021 · 9 comments
Closed

Use of sycl::half, with CL/sycl.hpp SYCL header included #149

sbalint98 opened this issue Dec 3, 2021 · 9 comments
Labels
question A request for more information or clarification

Comments

@sbalint98
Copy link
Contributor

In #143, the half data types were replaced with sycl::half, while including the CL/sycl.hpp SYCL header. According to the SYCL specification section 4.3, when that header is included all SYCL types should exist inside the ::cl::sycl namespace.

Unfortunately, this change also breaks compilation with hipSYCL, since sycl::half is not defined. In order to solve this problem I see three possible solutions:

  1. Use, like for all other SYCL types, the ::cl::sycl namespace for half as well
  2. Move to the ::sycl namespace in case of all SYCL types, and include the sycl/sycl.hpp header instead of CL/sycl.hpp
  3. Add a hipSYCL specific workaround similar to Add hipSYCL scope_handle and host_task #122 (comment)

I think the most consistent and least error-prone solution would be the first one. Can you give some feedback on what would be the preferred solution?

@sbalint98 sbalint98 added the question A request for more information or clarification label Dec 3, 2021
@sbalint98 sbalint98 changed the title Use of sycl::half, and included SYCL header Use of sycl::half, with CL/sycl.hpp SYCL header included Dec 3, 2021
@mmeterel
Copy link
Contributor

mmeterel commented Dec 3, 2021

@sbalint98 Thanks for bringing up this issue. I wonder if option 2 is a viable option. Did you get a chance to try it with hipSYCL and DPC++?
@mkrainiuk Do you see any issues with option 2?

@sbalint98
Copy link
Contributor Author

I'll have a look at option two and open a PR if it works

@sbalint98
Copy link
Contributor Author

sbalint98 commented Dec 3, 2021

It looks to me that the sycl/sycl.hpp header is not part of the dpc++ 2021.4.0 release (installed by the oneAPI base toolkit install script, version: 2021.4.0.3422). I am a bit confused since this file have been merged into the dpc++ GitHub repo here: intel/llvm#3290 in March already. Can you help me with what I might be missing here?

If you can confirm that the sycl/sycl.hpp is indeed not part of the current release of dpc++, I think we can exclude option 2.

EDIT:
Tested option number one for checking #140, if you agree I will open a PR...

@mmeterel
Copy link
Contributor

mmeterel commented Dec 6, 2021

It looks to me that the sycl/sycl.hpp header is not part of the dpc++ 2021.4.0 release (installed by the oneAPI base toolkit install script, version: 2021.4.0.3422). I am a bit confused since this file have been merged into the dpc++ GitHub repo here: intel/llvm#3290 in March already. Can you help me with what I might be missing here?

If you can confirm that the sycl/sycl.hpp is indeed not part of the current release of dpc++, I think we can exclude option 2.

EDIT: Tested option number one for checking #140, if you agree I will open a PR...

Any chance you can send your branch so I can take a quick look before opening a PR?

@mmeterel
Copy link
Contributor

mmeterel commented Dec 6, 2021

  1. se, like for all other SYCL types, the ::cl::sycl namespace for half as well

I looked at the 2021.4 release and indeed could not locate sycl/sycl.hpp. Sorry for the confusion with my comment. We can exclude option 2 as you suggested.

@sbalint98
Copy link
Contributor Author

Any chance you can send your branch so I can take a quick look before opening a PR?

Sure, have a look at this branch: https://github.com/sbalint98/oneMKL/tree/fix-qualification-ver2

This was referenced Jan 17, 2022
@mmeterel
Copy link
Contributor

@sbalint98 After looking through your branch, I would actually prefer a hipSYCL specific solution like in option 3. Could we bring that one back? Sorry for the inconvenience.

@sbalint98
Copy link
Contributor Author

No problem, added the change as requested to the PR #144

@mmeterel
Copy link
Contributor

Closing issue. It is handled with option 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question A request for more information or clarification
Projects
None yet
Development

No branches or pull requests

2 participants