Skip to content

[SYCL] Add sycl complex implementation #6309

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

Merged
merged 1 commit into from
Jun 28, 2022

Conversation

AidanBeltonS
Copy link
Contributor

This PR adds the sycl complex implementation of #6233. The implementation is based on llvm's libc++, but modified to use sycl builtins so it can be used within SYCL kernels.

This approach means that this can run on any backend which has implemented the math built-ins, therefore adding complex support for CUDA.

This PR includes a test to validate correct return types based on the proposal.

llvm-test-suite: intel/llvm-test-suite#1058

@steffenlarsen
Copy link
Contributor

I assume the heavy use of reserved identifiers is due to this being adjusted from the libc++ headers. Could you please change these to not use reserved identifiers? Preferably they should follow the LLVM Coding Standard.

Note that we are discussing changing our naming in #2981, with using reserved identifiers being one of the discussed option. However, for now please refrain from using these.

@steffenlarsen
Copy link
Contributor

I assume the heavy use of reserved identifiers is due to this being adjusted from the libc++ headers. Could you please change these to not use reserved identifiers? Preferably they should follow the LLVM Coding Standard.

I discussed this offline with @aelovikov-intel and he made the point that it could be useful to be able to diff this and the libc++ variant in the future. We also agreed that even if we remove the use of reserved identifiers in this, then optimally it would be as a separate PR so if #2981 ends up with us encouraging the use of reserved identifiers we can revert it easily.

To summarize, I am okay with leaving it as-is for now and potentially changing the identifiers separately. Maybe @againull or @bader have opinions on this.

@AidanBeltonS
Copy link
Contributor Author

I discussed this offline with @aelovikov-intel and he made the point that it could be useful to be able to diff this and the libc++ variant in the future. We also agreed that even if we remove the use of reserved identifiers in this, then optimally it would be as a separate PR so if #2981 ends up with us encouraging the use of reserved identifiers we can revert it easily.

Okay, I am not too concerned with being able to diff this and the libc++ variant, so I do not have issues with removing the reserved identifiers. I do agree that if the identifiers are removed it should be a seperate PR, to make it easy to undo.

@againull againull merged commit 1a03643 into intel:sycl Jun 28, 2022
@AidanBeltonS AidanBeltonS deleted the sycl_complex_impl branch June 29, 2022 08:55
steffenlarsen pushed a commit to intel/llvm-test-suite that referenced this pull request Aug 23, 2022
This PR adds testing for the experimental sycl complex implementation. 

It tests math functions, `abs`, `acos`, `asin`, `atan`, `acosh`, `asinh`, `atanh`, `arg`, `conj`, `cos`, `cosh`, `exp`, `log`, `log10`, `norm`, `polar`, `pow`, `proj`, `sin`, `sinh`, `sqrt`, `tanh`, and `tanh`. Additionally it tests the math operators `+`, `-`, `/`, `*` and their assign equivalents. It also tests stream operations with `sycl::stream`, `std::istream`, and `std::ostream`.

For simplicity the tests are broken into the test and a list of test cases. Each test is run for the complex type with `double`, `float`, and `sycl::half`. 

Results are compared to values from `std::complex`. Due to differences in error handling for the `pow` function between, libstdc++ and libc++(which the implementation is based on), testing of NAN's and INF's of pow is not done through comparison with `std::complex` but instead using expected values.

Depends on: intel/llvm#6309
aelovikov-intel pushed a commit to aelovikov-intel/llvm that referenced this pull request Mar 27, 2023
This PR adds testing for the experimental sycl complex implementation. 

It tests math functions, `abs`, `acos`, `asin`, `atan`, `acosh`, `asinh`, `atanh`, `arg`, `conj`, `cos`, `cosh`, `exp`, `log`, `log10`, `norm`, `polar`, `pow`, `proj`, `sin`, `sinh`, `sqrt`, `tanh`, and `tanh`. Additionally it tests the math operators `+`, `-`, `/`, `*` and their assign equivalents. It also tests stream operations with `sycl::stream`, `std::istream`, and `std::ostream`.

For simplicity the tests are broken into the test and a list of test cases. Each test is run for the complex type with `double`, `float`, and `sycl::half`. 

Results are compared to values from `std::complex`. Due to differences in error handling for the `pow` function between, libstdc++ and libc++(which the implementation is based on), testing of NAN's and INF's of pow is not done through comparison with `std::complex` but instead using expected values.

Depends on: intel#6309
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