[SYCL][Doc] Add SYCL marray complex extension proposal

[AidanBeltonS]

This PR adds an extension that proposes specializing the marray class to support the SYCL complex extension in #6233. The marray class is slightly modified, with some inapplicable operators removed for the specialization. No new functions or operators are added to the marray class.

Five overloaded free functions are added to provide complex functionality to the marray specialization; make_complex_marray, get_real/get_imag, and set_real/set_imag. They provide the functionality normally given by constructors and member functions real and imag.

Finally, all relevant complex math functions are extended to support this class.

[rolandschulz]

Should we really add multiple make_complex_marray overloads just for complex? Wouldn't it make more sense to add a general purpose generator expression mechanism? E.g. like the one of std::simd in parallelism TSv2.
If we add template<class G> marray(G&& gen) instead of writing make_complex_marray(ar, ai) (where ar, ai are marrays) one could write marray([&](auto i){return complex{ar[i], ai[i]};}. Isn't that simple enough?

[AidanBeltonS]

Applications using arrays of complex values will likely want an interface that performs similar operations to the std::complex interface. I.e. being able to construct a complex value with a real and imaginary vector component. While marray([&](auto i){return complex{ar[i], ai[i]};} would work, it is much more verbose than make_complex_marray(ar, ai), for something that is a simple element-wise constructor and it would be a significant departure from the simple std::complex constructors.

[rolandschulz]

Applications using arrays of complex values will likely want an interface that performs similar operations to the std::complex interface.

I can see that. The question is why should that need be met by SYCL (initially as extension)?I don't think we want to add all math functionality applications reasonable want to have and slowly end up with an API size similar to libraries like Eigen. To keep the API reasonable small we should only have in SYCL what can be considered fundamental building blocks and/or can't be implemented (efficiently) as library. And I don't think make_complex_marray qualifies as such.

[AidanBeltonS]

So regarding your point, I think that if these functions were made member functions it may be less intrusive and awkward in the spec. I have updated the proposal to specialize marray and made these functions members of the class. Let me know your thoughts on this approach and if this addresses some of your concerns.

[rolandschulz]

Same as for creating, do we need this? If we can't instead add e.g. map/transform, then get_real(a) would become map(a, [](auto e){return e.real();}.

[AidanBeltonS]

My opinion is the same as above. This is more verbose and when dealing with vector math it would look much more messy.

[keryell]

I have the feeling that the classical operations could be constexpr, at least for integral types.

[AidanBeltonS]

I have the feeling that the classical operations could be constexpr, at least for integral types.

Hi, thanks for the comment, could you clarify what classical operations you are referring to, is this the arithmetic operations +,-, etc or classical complex functions like real/imag?
Additionally what specifically do you mean by integral types, is this when the underlying complex value type is an integer ex: marray<complex<int>> or are you referring to the integral_sequences?

[keryell]

I have the feeling that the classical operations could be constexpr, at least for integral types.

Hi, thanks for the comment, could you clarify what classical operations you are referring to, is this the arithmetic operations +,-, etc or classical complex functions like real/imag? Additionally what specifically do you mean by integral types, is this when the underlying complex value type is an integer ex: marray<complex<int>> or are you referring to the integral_sequences?

I was thinking to simple operations like -, +, etc. But actually whatever we can. :-)
https://en.cppreference.com/w/cpp/concepts/integral
For floating point and complex types, there might be some differences between constexpr and runtime computation because the binary representation is not part of the standard.
That said, SYCL makes a lot of assumptions inherited from OpenCL and SPIR about data representations, so perhaps we can be optimistic and make everything constexpr?

[AidanBeltonS]

I was thinking to simple operations like -, +, etc. But actually whatever we can. :-)
https://en.cppreference.com/w/cpp/concepts/integral
For floating point and complex types, there might be some differences between constexpr and runtime computation because the binary representation is not part of the standard.
That said, SYCL makes a lot of assumptions inherited from OpenCL and SPIR about data representations, so perhaps we can be optimistic and make everything constexpr?

So for operators, it seems bad practice to have the complex specialization of the marray type use different keywords to the generic marray type. Perhaps these operators should specify constexpr. Still, that change and discussion should probably be for the general marray class, not the complex extension.

That being said I think real and imag, just the overloads that return values, should be constexpr as this would match the std::complex interface. Additionally, I think it is safe to have this be a general change, not just for the integral type. I have updated this proposal to reflect the keyword change.

[jle-quel]

@TApplencourt The problem that we were talking about with the ambiguous getter and setters, as well as the free functions, has already been discussed here.

[TApplencourt]

Sorry for chiming in for the constructor, getter, setter discussion. IMO for this work, we should be supper boring and follow std::complex. So I concur with @rolandschulz

• The getter should be named real and imag so I can do std::marray<complex<T>,N> A ; A.real() and should not take any arguments.
• The setter should just reuse &= with the same semantic as std::complex. If I pass a marray<T> it will set only the real part, the imaginary part will be 0.
• The constructor should mimic the std::complex (take only marray as arguments). No need for free function.

If people want to set/get an individual element of the marray, they can just use the [] operator and wrote their own wrapper. I think we should try to keep this API simple :)

[keryell]

Do you have some code samples using these std::integer_sequence interfaces?
I am unsure there is any use of user-facing std::integer_sequence interface in the C++ standard library.
You could just use .swizzle<...>() on the final vector and rely on your peephole optimizer or use the swizzle on each component marray at the first place.
I have created KhronosGroup/SYCL-Docs#320 along this.

[jle-quel]

Apart from this marray's test, I'm not aware of others using these constructors and getters.

[jle-quel]

Should we organize a meeting where we could talk about our different opinions, and conclude on what we should do?

Otherwise, do we agree with @TApplencourt and @rolandschulz, plus the removal of the constructors and getters using std::integer_sequenceas @keryell mentioned?

[jle-quel]

Open's conversation to close before being able to do the final review:

• constructor, getter, setter discussion
• relevance of std::integer_sequence

[TApplencourt]

I'm open to having any other meetings if that help moving forward.

[TApplencourt]

Should we vote to move this issue forward?
I'm not sure democracy is the best solution to solve technical issues, but it should make it moving.

1: ❤️ Minimal API ( removal of most of the getter, setter, and integral sequence, ...)
2: 🚀 Keep it as is (integral sequence, set_real, ...)

Please use "emoji reaction" to vote (1 hear, 2 rocket).
And Happy new year :)

[TApplencourt]

The people voted! Can we modify the PR to remove the API and you the "light" version? Thanks!

[jle-quel]

Due to the new PR introducing the restructured marray specialization proposal, I will close this PR.

Please refer to the new PR #8852

[github-actions]

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]

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