llvm icon indicating copy to clipboard operation
llvm copied to clipboard

[SYCL] Introduce sycl complex marray specialization

Open jle-quel opened this issue 2 years ago • 13 comments

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.

jle-quel avatar Mar 14 '23 16:03 jle-quel

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.

jle-quel avatar Mar 28 '23 13:03 jle-quel

Is there anything else you wanted me to update regarding this PR @steffenlarsen ?

jle-quel avatar Jun 21 '23 10:06 jle-quel

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.

jle-quel avatar Aug 07 '23 10:08 jle-quel

Gentle ping to get some attention into this PR 👍 @aelovikov-intel @steffenlarsen

jle-quel avatar Sep 12 '23 14:09 jle-quel

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.

aelovikov-intel avatar Sep 20 '23 15:09 aelovikov-intel

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.

jle-quel avatar Sep 22 '23 10:09 jle-quel

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.

steffenlarsen avatar Sep 22 '23 10:09 steffenlarsen

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.

jle-quel avatar Feb 08 '24 11:02 jle-quel

Have you seen this https://github.com/intel/llvm/pull/12786 @steffenlarsen?

jle-quel avatar Feb 26 '24 09:02 jle-quel

Have you seen this #12786 @steffenlarsen?

I missed that! Thanks for pointing it out. 😄

steffenlarsen avatar Feb 26 '24 09:02 steffenlarsen

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?

jle-quel avatar May 13 '24 12:05 jle-quel

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.

steffenlarsen avatar May 13 '24 15:05 steffenlarsen

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

jle-quel avatar May 20 '24 09:05 jle-quel