Add initial marray implementation #12

AidanBeltonS · 2022-07-28T14:20:03Z

This PR adds the marray<complex, N> specialization. The marray complex specialization is added using a mixture of free functions and deleting non applicable operators. This causes the marray interface to deviate slightly for the original sycl interface. The marray class is added in the sycl namespace to allow for proper specialization.

This additionally adds tests for the operators, interface, and free functions.

AidanBeltonS · 2022-08-08T08:15:14Z

@TApplencourt this PR is now ready for review, testing of the marray type has been added.

AerialMantis · 2022-10-06T11:18:45Z

@TApplencourt friendly ping to request a review on this

TApplencourt · 2022-10-06T14:19:50Z

Oh thanks a lot for the ping. I totally miss Aidan previous message! Sorry about that

AerialMantis · 2022-10-06T15:13:43Z

No problem :)

…ommit)

TApplencourt · 2022-10-25T22:35:58Z

Sorry, Didn't forget about this! Will find the time to review this week

Update sycl_complex_marray branch

TApplencourt · 2022-11-04T16:15:23Z

include/sycl_ext_complex.hpp

+sycl::marray<complex<T>, NumElements>
+make_complex_marray(const sycl::marray<T, NumElements> &real,
+                    const sycl::marray<T, NumElements> &imag) {
+  sycl::marray<complex<T>, NumElements> rtn;


Stupid question, but can we not have a new array constructor? (and not a free function)

TApplencourt · 2022-11-04T16:16:20Z

include/sycl_ext_complex.hpp

+template <class T, std::size_t NumElements>
+sycl::marray<complex<T>, NumElements>
+make_complex_marray(const sycl::marray<T, NumElements> &real, const T &imag) {
+  sycl::marray<complex<T>, NumElements> rtn;
+
+  for (std::size_t i = 0; i < NumElements; ++i) {
+    rtn[i].real(real[i]);
+    rtn[i].imag(imag);
+  }
+
+  return rtn;
+}
+
+template <class T, std::size_t NumElements>
+sycl::marray<complex<T>, NumElements>
+make_complex_marray(const T &real, const sycl::marray<T, NumElements> &imag) {
+  sycl::marray<complex<T>, NumElements> rtn;
+
+  for (std::size_t i = 0; i < NumElements; ++i) {
+    rtn[i].real(real);
+    rtn[i].imag(imag[i]);
+  }
+


Do we really want those? They sound super ugly... (and lead to bug in user-code)

Yes, we can remove all make_complex_marray implementations.
I'll generate the marray test cases using the GENERATE macro from catch2.

// Test cases auto init_re = GENERATE(test_marray<T, NumElements>{ 1.0, 4.42, -3, 4.0, 2.02, inf_val<T>, inf_val<T>, 2.02, nan_val<T>, nan_val<T>, inf_val<T>, nan_val<T>, inf_val<T>, nan_val<T>}); auto init_im = GENERATE(test_marray<T, NumElements>{ 1.0, 2.02, 3.5, -4.0, inf_val<T>, 4.42, nan_val<T>, 4.42, nan_val<T>, nan_val<T>, inf_val<T>, nan_val<T>, inf_val<T>, nan_val<T>}); sycl::marray<sycl::ext::cplx::complex<T>, NumElements> cplx_input = sycl::ext::cplx::make_complex_marray(init_re.get(), init_im.get());

becomes

auto cplx_input = GENERATE(sycl::marray<sycl::ext::cplx::complex<T>, NumElements>{ sycl::ext::cplx::complex<T>{1.0, 1.0}, sycl::ext::cplx::complex<T>{4.42, 2.02}, sycl::ext::cplx::complex<T>{-3, 3.5}, sycl::ext::cplx::complex<T>{4.0, -4.0}, sycl::ext::cplx::complex<T>{2.02, inf_val<T>}, sycl::ext::cplx::complex<T>{inf_val<T>, 4.42}, sycl::ext::cplx::complex<T>{inf_val<T>, nan_val<T>}, sycl::ext::cplx::complex<T>{2.02, 4.42}, sycl::ext::cplx::complex<T>{nan_val<T>, nan_val<T>}, sycl::ext::cplx::complex<T>{nan_val<T>, nan_val<T>}, sycl::ext::cplx::complex<T>{inf_val<T>, inf_val<T>}, sycl::ext::cplx::complex<T>{nan_val<T>, nan_val<T>}, sycl::ext::cplx::complex<T>{inf_val<T>, inf_val<T>}, sycl::ext::cplx::complex<T>{nan_val<T>, nan_val<T>}, });

Thanks! I would like to keep this namespace as small as possible. Extending CATCH2 Macro sound like a good path :)

TApplencourt · 2022-11-04T16:43:55Z

include/sycl_ext_complex.hpp

+
+// Get
+
+template <class T, std::size_t NumElements>


Should we have those free functions?
Why nover overlead .real() on marray to return those?

I can also remove the Getter and Setter.
There is no real neither imag methods for the specialisation of the marray

Would you prefer not to have these functionalities on marray (just delete it)
Or implement real and imag methods on the marray specialization?

real() -> sycl::marray<T, NumElements> // getter imag() -> sycl::marray<T, NumElements> // getter real(sycl::marray<T, NumElements>) // setter imag(sycl::marray<T, NumElements>) // setter

@TApplencourt ☝️ ?

Or implement real and imag methods on the marray specialization?

This sound cleaner to me! (sorry for the delay)

Here's the list of all getters and setters proposed in this PR:

// getter functions sycl::marray<T, NumElements> real(); sycl::marray<T, NumElements> imag(); T real(std::size_t index); T imag(std::size_t index); template <std::size_t... I> auto constexpr real(std::integer_sequence<std::size_t, I...> int_seq); template <std::size_t... I> auto constexpr imag(std::integer_sequence<std::size_t, I...> int_seq) // setter functions void real(const sycl::marray<T, NumElements> &values); void imag(const sycl::marray<T, NumElements> &values); void real(const T &value); void imag(const T &value); void real(std::size_t index, const T &value); void imag(std::size_t index, const T &value);

One problem with overloading the real and imag for both getters and
setters is that we will create an ambiguous situation since overload resolution
doesn't depend on return values, but only arguments.

getter: T real(std::size_t index);

setter: void real(const T &value);

What we could do is to keep the getX and setX and overload them, or
other variants like implicit (ex: real) for getter and explicit
(ex: setReal) for setters.

What do you think?

I think keeping real() / imag() as a getter is a good idea (it mimic std::complex )
(By the way, do we need real(std::size_t index) I guess we can always do a[i].real()?)

For setting, this is a good question... Can we just do like std::complex? Aka, just use &=?

If the user passes a std::marray<complex> we set both real and imaging, and if just a std::marray<float>, we set just the real part. If people want to set a particular value a[i]=23 should do the trick.

What do you think?

Since we are starting to rethink the design of the marray implementation, I believe we should do this as part of the DPC++ and Khronos proposal.

I'll update the marray's DPC++ proposal, and I'll ping you there so we can continue our discussion together with the community as well.

Then, we'll take back the decision made in the proposal, and I'll update the implementation.

Good idea; thanks a lot!

TApplencourt · 2022-11-04T16:45:21Z

Thanks a lot, and sorry for all the ~~stupid~~ naive questions!

jle-quel · 2022-11-09T11:55:28Z

tests/test_operator_marray_complex.cpp

+                     test_marray<T, NumElements> init_im1,                     \
+                     test_marray<T, NumElements> init_re2,                     \
+                     test_marray<T, NumElements> init_im2) {                   \
+    auto *cplx_inout = sycl::malloc_shared<                                    \


Memory leak, cplx_inout is not freed

Fix memory leak

jle-quel · 2023-02-06T10:06:10Z

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

Please refer to the new PR #33,

aidan.belton added 3 commits August 5, 2022 13:18

Add marray implementation

9758065

Update test helper

61c65da

Add marray testing

2c06f31

AidanBeltonS force-pushed the sycl_complex_marray branch from a6a11d4 to 2c06f31 Compare August 5, 2022 12:19

Add integer sequence overloads to free functions

4a6c4f8

AidanBeltonS marked this pull request as ready for review August 8, 2022 08:12

aidan.belton added 7 commits August 11, 2022 15:00

Update template type checking

627bb96

Add additional test_valid_types overload

ed22c49

Add type checking to device code

a05991e

Remove unneccessary else

06e72b1

Add wait to stream test

20e2669

Update pow test to take two inputs

873ef2b

Improve marray complex test values

8e0db5a

AidanBeltonS changed the title ~~[WIP] Add initial marray implementation~~ Add initial marray implementation Aug 22, 2022

jle-quel added 4 commits October 21, 2022 10:30

pull upstream; fix conflicts; update marray testing to use Catch2

d6fbd03

update marray testing to use Catch2

7f38c63

increase tol_multiplier for acos(h) and asin(h) tests

8199298

increase tol_multiplier for acos(h) and asin(h) tests (fix previous c…

43c71fb

…ommit)

jle-quel mentioned this pull request Oct 21, 2022

Update sycl_complex_marray branch AidanBeltonS/SyclCPLX#1

Merged

Merge pull request #1 from jle-quel/jle-quel/fix-conflicts

37f19e0

Update sycl_complex_marray branch

TApplencourt reviewed Nov 4, 2022

View reviewed changes

jle-quel reviewed Nov 9, 2022

View reviewed changes

jle-quel and others added 2 commits November 9, 2022 12:56

fix memory leak

725dd39

Merge pull request #2 from jle-quel/jle-quel/fix-memory-leak

3c05fda

Fix memory leak

TApplencourt closed this May 31, 2023

Add initial marray implementation #12

Add initial marray implementation #12

Uh oh!

Conversation

AidanBeltonS commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AidanBeltonS commented Aug 8, 2022

Uh oh!

AerialMantis commented Oct 6, 2022

Uh oh!

TApplencourt commented Oct 6, 2022

Uh oh!

AerialMantis commented Oct 6, 2022

Uh oh!

TApplencourt commented Oct 25, 2022

Uh oh!

TApplencourt Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

TApplencourt Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jle-quel Nov 8, 2022

Choose a reason for hiding this comment

Uh oh!

TApplencourt Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

TApplencourt Nov 4, 2022

Choose a reason for hiding this comment

Uh oh!

jle-quel Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jle-quel Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TApplencourt Nov 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jle-quel Nov 14, 2022

Choose a reason for hiding this comment

Uh oh!

TApplencourt Nov 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jle-quel Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TApplencourt Nov 16, 2022

Choose a reason for hiding this comment

Uh oh!

TApplencourt commented Nov 4, 2022

Uh oh!

jle-quel Nov 9, 2022

Choose a reason for hiding this comment

Uh oh!

jle-quel commented Feb 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AidanBeltonS commented Jul 28, 2022 •

edited

Loading

TApplencourt Nov 4, 2022 •

edited

Loading

jle-quel Nov 8, 2022 •

edited

Loading

jle-quel Nov 10, 2022 •

edited

Loading

TApplencourt Nov 11, 2022 •

edited

Loading

TApplencourt Nov 15, 2022 •

edited

Loading

jle-quel Nov 16, 2022 •

edited

Loading