llvm
llvm copied to clipboard
[SYCL] Introduce sycl complex marray specialization
Following the introduction of sycl::ext::oneapi::experimental::complex (https://github.com/intel/llvm/pull/8068), which adds the support of complex for SYCL, this PR allows for adding new complex math features.
The specialization of sycl::marray to add support for storing complex numbers in arrays is proposed as well as overloading the existing math functions to support complex marrays and adding member functions to simplify accessing, setting marray values, and constructing complex marrays.
The latest commits are the propagation of this PR https://github.com/intel/llvm-test-suite/pull/1661 which was on the recently archived llvm-test-suite repository.
Is there anything else you wanted me to update regarding this PR @steffenlarsen ?
Latest updates:
- SYCL end-to-end test suites for Windows were failing due to special test cases that can't be checked for equality with other complex implementations.
- This same test suite for Windows, generated some ambiguity errors. (Window's complex implementation for windows is unspecified for other types than
float,double,ldouble).
Both failures have been fixed.
Finally, I believe that I addressed all your feedback @aelovikov-intel @steffenlarsen. If that's not the case, let me know; otherwise, we can move forward with this PR.
Gentle ping to get some attention into this PR 👍 @aelovikov-intel @steffenlarsen
Gentle ping to get some attention into this PR 👍 @aelovikov-intel @steffenlarsen
I'll leave it to @steffenlarsen who's done most of the review here.
I just have one request. Am I right that this is blocked by #8852?
It's not really blocked, since there is no build dependency between the implementation and the proposal, however, yes they represent the same thing so they should be merged together.
Typically we would want the extension approved and merged before we consider merging the implementation of it. As such https://github.com/intel/llvm/pull/8852 should be first, I would argue.
The specification for marray has been merged as an extension to the complex specification.
PR: https://github.com/intel/llvm/pull/11792
Pinging @steffenlarsen , since this PR is not blocked anymore by the specification.
Have you seen this https://github.com/intel/llvm/pull/12786 @steffenlarsen?
Have you seen this #12786 @steffenlarsen?
I missed that! Thanks for pointing it out. 😄
With the latest changes made to the specification, I believe the two comments that you left @steffenlarsen about the deleted operators are resolved. The specification explicitly marked the operators as deleted. The implementation just does not have to define them, nor delete them. Are we okay with that?
The last comment that needs to be resolved before this PR can get merged is the one at "https://github.com/intel/llvm/pull/8647#discussion_r1484301864".
I'm not sure I fully understand what needs to be done. Could you please be more specific?
The last comment that needs to be resolved before this PR can get merged is the one at "#8647 (comment)".
I'm not sure I fully understand what needs to be done. Could you please be more specific?
We refactored the builtins, so the comment was about making these new builtins similar in definition to the new structure. See https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/detail/builtins/math_functions.inc and https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/detail/builtins/helper_macros.hpp.
The last comment that needs to be resolved before this PR can get merged is the one at "#8647 (comment)". I'm not sure I fully understand what needs to be done. Could you please be more specific?
We refactored the builtins, so the comment was about making these new builtins similar in definition to the new structure. See https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/detail/builtins/math_functions.inc and https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/detail/builtins/helper_macros.hpp.
I've updated the implementation of the marray builtin to follow the new structure of the builtins. However, it is a bit more convoluted since we are merging a common interface but with different arguments and logic within that function.
I've added some comments to help understand the macros and how to create a new builtin if needed in the future.
Let me know what you think @steffenlarsen