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.

Sign up for GitHub

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

Jump to bottom

[SYCL][DOC] Introduce sycl-marray-complex specialization proposal #8852

Closed
wants to merge 2 commits into from
Closed

[SYCL][DOC] Introduce sycl-marray-complex specialization proposal #8852

wants to merge 2 commits into from

Conversation

jle-quel
Copy link
Contributor

Following the proposal for sycl::ext::oneapi::experimental::complex and its implementation (#8068), which adds the support of complex for SYCL, this proposal allows for adding new complex math features.

This PR proposes the specialization of sycl::marray to add support for storing complex numbers in arrays.

@jle-quel
Copy link
Contributor Author

The implementation of the sycl::marray specialization for complex can be found here #8647

@jle-quel jle-quel mentioned this pull request Mar 29, 2023
Closed
@jle-quel jle-quel marked this pull request as ready for review March 29, 2023 09:56
@jle-quel jle-quel requested a review from a team as a code owner March 29, 2023 09:56
@jle-quel jle-quel requested a review from steffenlarsen April 27, 2023 08:32
rolandschulz
rolandschulz reviewed May 4, 2023
View reviewed changes
sycl/doc/extensions/proposed/sycl_ext_oneapi_complex_marray.asciidoc
marray<T, NumElements> imag() const;

// subscript operator
reference operator[](std::size_t i);
Copy link
Contributor

Choose a reason for hiding this comment

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

This requires that the internal data representation is an array of complex. Are we reasonable sure no implementation wants the data storage to be one array for real and one for imag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please, let me know if I misunderstood your feedback, but that's what the sycl::marray specialization for sycl::complex is about.
If needed, a user could use the real() or imag() method to let him work on the data split.

@TApplencourt, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is already an implementation of this complex specialization of marray within this repo https://github.com/argonne-lcf/SyclCPLX.
No issues have been raised about having an array of complex rather than two arrays for real and imag.

Also, this PR is for the proposed directory and later will be moved onto the experimental.
We'll not be tied to this proposal implementation.

I believe it is conceivable to have an array of complex for this proposal as a start, and if needed we'll change the structure layout.

@jle-quel jle-quel requested a review from gmlueck August 7, 2023 13:44
@jle-quel
Copy link
Contributor Author

Gentle ping to get some attention into this PR 👍 @rolandschulz @steffenlarsen @gmlueck

steffenlarsen
steffenlarsen reviewed Sep 21, 2023
View reviewed changes
Copy link
Contributor

@steffenlarsen steffenlarsen left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, but I would like @rolandschulz to confirm that his concerns have been addressed.

@steffenlarsen steffenlarsen mentioned this pull request Sep 21, 2023
Closed
gmlueck
gmlueck reviewed Sep 21, 2023
View reviewed changes
@gmlueck
Copy link
Contributor

gmlueck commented Sep 21, 2023

Ack! Sorry, I just noticed that my previous comments never got submitted. (They were stuck as "pending" in the PR.) Sorry about the delay.

@gmlueck
Copy link
Contributor

gmlueck commented Sep 25, 2023

One other thought ... Why should this be a separate extension rather than an expansion of the existing sycl_ext_oneapi_complex?

Of course you can argue that more granular extensions give implementors greater freedom to choose what they implement, but more extensions makes life harder for application developers who want to write portable code.

Consider an implementor who already implements sycl_ext_oneapi_complex. Is there some reason that implementor might not be able to implement these marray functions? It seems like it's just a matter of some extra code on top of what they need already for sycl_ext_oneapi_complex.

@jle-quel
Copy link
Contributor Author

jle-quel commented Sep 25, 2023 •
edited
Loading

One other thought ... Why should this be a separate extension rather than an expansion of the existing sycl_ext_oneapi_complex?

Of course you can argue that more granular extensions give implementors greater freedom to choose what they implement, but more extensions makes life harder for application developers who want to write portable code.

Consider an implementor who already implements sycl_ext_oneapi_complex. Is there some reason that implementor might not be able to implement these marray functions? It seems like it's just a matter of some extra code on top of what they need already for sycl_ext_oneapi_complex.

You are right, the better definition of that proposal is an expansion of the existing complex. Bringing other types which handle complex and overloads for the complex's math functions to support this new type.

No, there is no reason that an implementor might not be able to implement these marray functions, considering that he has an marray implementation.

@steffenlarsen
Copy link
Contributor

@gmlueck & @jle-quel - What are the next steps here?

@jle-quel
Copy link
Contributor Author

jle-quel commented Oct 19, 2023 •
edited
Loading

@gmlueck & @jle-quel - What are the next steps here?

On my side, I got back to work on this today.
I'll update the "getter" real/imag API as discussed with Greg, as well as the other texts/typos feedback.

Regarding the implementation of this proposal (#8647) I got back to it today as well, and I'll update the getter API, and also split the single header files into multiples as we discuss

The next steps I'm working towards and hoping for would be:

  • The proposal would be up to date with the latest feedback.
  • I am still unsure about the separate extension versus expansion (Don't have a strong opinion on it).
  • The implementations would be up to date as well with the implementation.
  • Finally, a final review before we might merge the marray's complex specialization (doc and impl)

@gmlueck
Copy link
Contributor

gmlueck commented Oct 19, 2023

I am still unsure about the separate extension versus expansion (Don't have a strong opinion on it).

I'd prefer an expansion of the existing extension unless we have a good reason to make a separate extension.

@jle-quel jle-quel mentioned this pull request Oct 20, 2023
Merged
@jle-quel
Copy link
Contributor Author

jle-quel commented Nov 2, 2023 •
edited
Loading

Since we are moving away from having a separate extension, I'll create another PR to expand the complex extension with the modifications asked here.
I'll link the two together for the future reader to know where the discussion went and will close this PR.

@jle-quel jle-quel mentioned this pull request Nov 6, 2023
Merged
dm-vodopyanov pushed a commit that referenced this pull request Feb 7, 2024
@jle-quel
[SYCL][DOC] Expand complex extension with complex support for sycl::m…
32f5191 
…array (#11792)

This PR is the updated version of the "complex support for sycl::marray"
extension, which used to be a separate extension.
However, after discussing whether it should be a separate extension
versus an expansion (#8852), the
complex expansion has been decided.

Following the proposal for `sycl::ext::oneapi::experimental::complex`
which adds the support of complex for SYCL, this expansion proposes to
incorporate complex support for `sycl::marray` and new complex math
features.

Implementation: #8647
@jle-quel jle-quel closed this Feb 7, 2024
@jle-quel jle-quel deleted the jle-quel/introduce-sycl_complex_marray-proposal branch February 7, 2024 11:22
@jle-quel jle-quel mentioned this pull request Nov 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rolandschulz rolandschulz rolandschulz left review comments

@gmlueck gmlueck gmlueck left review comments

@steffenlarsen steffenlarsen steffenlarsen left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jle-quel @gmlueck @steffenlarsen @rolandschulz