Skip to content

Conversation

@mmeterel
Copy link
Contributor

@mmeterel mmeterel commented May 17, 2022

Description

This PR is refactoring sycl header file by using sycl/sycl.hpp whenever it is available, like in hipSYCL case.

For DPC++ and LLVM, we will start using sycl/sycl.hpp whenever it becomes available.

This PR is fixing #193 and partially fixing #198

Checklist

All Submissions

  • Do all unit tests pass locally?
    Tested the below combinations

gen9, mklcpu, mklgpu: blas: all pass
gen9: mklcpu, mklgpu: lapack: I see errors which are not related to this PR. They also exist in develop.
gen9: mklcpu, mklgpu: rng: all pass

nvidia, mklcpu, cublas: blas: all pass
nvidia, mklcpu, cusolver: lapack: all pass
nvidia, mklcpu, curand: rng: all pass

with hipSYCL:
amd, mklcpu, rocblas: blas: all pass
amd, mklcpu, rocrand: rng: all pass except 4 tests. These failures are due to ROCM version. Will be fixed in ROCM 5.1.

  • Have you formatted the code using clang-format?

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.

Looks good. Are there plans for DPC++ to change to sycl/sycl.hpp? Is that part of some SYCL standard?

Minor: did the formatting changes come from clang-format? Is there some version change in clang-format that led to these changes?

Copy link
Contributor

@sknepper sknepper left a comment

Choose a reason for hiding this comment

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

LGTM as well! Thanks!

@mmeterel
Copy link
Contributor Author

Looks good. Are there plans for DPC++ to change to sycl/sycl.hpp? Is that part of some SYCL standard?

Minor: did the formatting changes come from clang-format? Is there some version change in clang-format that led to these changes?

Yes, DPCPP will move to sycl/sycl.hpp as well. It is part of SYCL spec. LLVM compiler already has sycl/sycl.hpp, but it simply points to CL/sycl.hpp.

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!

@mmeterel
Copy link
Contributor Author

Looks good. Are there plans for DPC++ to change to sycl/sycl.hpp? Is that part of some SYCL standard?
Minor: did the formatting changes come from clang-format? Is there some version change in clang-format that led to these changes?

Yes, DPCPP will move to sycl/sycl.hpp as well. It is part of SYCL spec. LLVM compiler already has sycl/sycl.hpp, but it simply points to CL/sycl.hpp.

AFAIK, no change in clang-format version. My guess is, those files were not following clang-format initially.

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