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

[SYCL] Introduce sycl complex marray specialization #8647

Conversation

jle-quel
Copy link
Contributor

@jle-quel jle-quel commented Mar 14, 2023

Following the introduction of sycl::ext::oneapi::experimental::complex (#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 jle-quel temporarily deployed to aws March 14, 2023 17:57 — with GitHub Actions Inactive
@jle-quel jle-quel temporarily deployed to aws March 14, 2023 19:21 — with GitHub Actions Inactive
@jle-quel
Copy link
Contributor Author

The latest commits are the propagation of this PR intel/llvm-test-suite#1661 which was on the recently archived llvm-test-suite repository.

@jle-quel jle-quel temporarily deployed to aws March 28, 2023 13:43 — with GitHub Actions Inactive
@jle-quel jle-quel temporarily deployed to aws March 28, 2023 15:16 — with GitHub Actions Inactive
@jle-quel jle-quel marked this pull request as ready for review March 29, 2023 09:55
@jle-quel jle-quel requested a review from a team as a code owner March 29, 2023 09:55
@jle-quel jle-quel requested a review from steffenlarsen April 27, 2023 08:32
@jle-quel jle-quel temporarily deployed to aws May 3, 2023 10:09 — with GitHub Actions Inactive
@jle-quel jle-quel temporarily deployed to aws May 3, 2023 10:09 — with GitHub Actions Inactive
@jle-quel
Copy link
Contributor Author

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

@jle-quel jle-quel temporarily deployed to aws June 27, 2023 10:04 — with GitHub Actions Inactive
@jle-quel jle-quel temporarily deployed to aws June 27, 2023 11:05 — with GitHub Actions Inactive
@jle-quel
Copy link
Contributor Author

Have you seen this #12786 @steffenlarsen?

@steffenlarsen
Copy link
Contributor

Have you seen this #12786 @steffenlarsen?

I missed that! Thanks for pointing it out. 😄

@jle-quel jle-quel temporarily deployed to WindowsCILock May 6, 2024 14:22 — with GitHub Actions Inactive
@jle-quel jle-quel temporarily deployed to WindowsCILock May 6, 2024 15:12 — with GitHub Actions Inactive
@jle-quel jle-quel temporarily deployed to WindowsCILock May 8, 2024 08:25 — with GitHub Actions Inactive
@jle-quel jle-quel temporarily deployed to WindowsCILock May 8, 2024 08:59 — with GitHub Actions Inactive
@jle-quel
Copy link
Contributor Author

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 "#8647 (comment)".

I'm not sure I fully understand what needs to be done. Could you please be more specific?

@steffenlarsen
Copy link
Contributor

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.

@jle-quel
Copy link
Contributor Author

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

Copy link
Contributor

This pull request is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be automatically closed in 30 days.

@github-actions github-actions bot added the Stale label Nov 20, 2024
Copy link
Contributor

This pull request was closed because it has been stalled for 30 days with no activity.

@github-actions github-actions bot closed this Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants