Introduction of marray's complex specialization #33
Conversation
|
See this PR AidanBeltonS/llvm#2 for reference, which will update the in-review proposal intel/llvm#6550 |
include/sycl_ext_complex.hpp
Outdated
| #define _SYCL_EXT_CPLX_FAST_MATH | ||
| #endif | ||
|
|
||
| #define _SYCL_BEGIN_NAMESPACE namespace sycl { |
There was a problem hiding this comment.
I think we maybe need to be careful with that, as we did for the other namespace, because of hipsycl where the namespace sycl will just be an alias to cl::so we will need to extent the base namespace and not sycl
There was a problem hiding this comment.
Just so we are on the same page, do you mean this?
#ifndef _SYCL_CPLX_NAMESPACE
#ifdef __HIPSYCL__
#define _SYCL_CPLX_NAMESPACE hipsycl::sycl::ext::cplx
#else
#define _SYCL_CPLX_NAMESPACE sycl::ext::cplx
#endif
#endif
So defining _SYCL_BEGIN_NAMESPACE to sycl if __HIPSYCL__ does not exist, else hipsycl::sycl
There was a problem hiding this comment.
Modified the namespace to also handle hypsycl, is that what you had in mind?
include/sycl_ext_complex.hpp
Outdated
| template <typename T, std::size_t NumElements> | ||
| class marray<sycl::ext::cplx::complex<T>, NumElements> { | ||
| private: | ||
| using DataT = sycl::ext::cplx::complex<T>; |
There was a problem hiding this comment.
Can we rename DataT do ComplexDataT or ComplexT just for sake of clarity?
I Was confused at the beginning between T and dataT
There was a problem hiding this comment.
Updated the type name
include/sycl_ext_complex.hpp
Outdated
| } | ||
|
|
||
| // subscript operator | ||
| reference operator[](std::size_t index) { return MData[index]; } |
There was a problem hiding this comment.
We use i in the foor loop on top, I guess we we can use i for index everywhere
There was a problem hiding this comment.
Updated the variable name
|
Thanks a lot for the PR; really sorry for the delay! |
|
Really sorry :( Merged! |
|
It looks like this never passed the CI? |
|
Yep. But it build, so I thought it was ok. |
|
This PR has been tested on opencl::cpu, opencl::gpu and nvidia::gpu. From what I remember, the error came from the CI regarding how DPC++ is installed. |
|
Also, there was as well, a problem of the clang-format test, which tested with two versions of clang-format; both refusing the formatting of the other |
Which gpu hardware did you run on? For me many tests fail on L0 Intel Gen9, which is not ideal, but perhaps not highest priority to fix. Waiting in queue to test on PVC, queues have been slow lately. Re the clang-format nonesense, I disabled the -9 test. Ideally we would have a .clang-format that would make it work the same across versions, but in practice that is a pain; unfortunately they do not prioritize stability in formatting, so it's a loosing battle. |
|
Sorry if I sound grumpy - just trying to get this repo in a better tested state, the lack of consistent CI (and no GPU CI) has been a problem for all of us. I think you may have tested on different systems than I have access to, so any info you can remember is helpful. I want to track down if/when things did pass and then add back changes from there to see what broke things. Not so much concerned with CI which was not mature enough, just having all tests pass with a manual run on GPU is what I am looking for as a baseline. |
|
I understand and share your aim. |
3 participants
This PR adds the
marray<complex, N>specialization. Themarraycomplex specialization is added using a mixture of free functions and deleting non applicable operators. This causes themarrayinterface to deviate slightly for the original sycl interface. Themarrayclass is added in the sycl namespace to allow for proper specialization.This additionally adds tests for the operators, interface, and free functions.