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

Update complex implementation and tests #25

Merged

Conversation

jle-quel
Copy link
Contributor

@jle-quel jle-quel commented Dec 12, 2022

This PR updates the complex implementation as well as the tests.

From the POV of the complex's user, the interface does not change.

The modification made to the implementation are:

  • the operators are now declared as hidden friends.
  • encapsulation of helper functions of the implementation (only expose what needs to be)
  • using metaprogramming to specialize the complex when the trait genfloat is true, which removes the duplication of operators
  • using metaprogramming to specialize different math operations when the trait genfloat is true, which removes some boilerplate code
  • add the missing attribute to different functions
  • remove const and ref keywords on value_type
  • add missing enable_if on free functions

The modifications made to the tests are:

  • fix different typos
  • add more tests

@jle-quel
Copy link
Contributor Author

I did not modify the synopsis yet. I'll change it when we are satisfied with these changes.
For the sake of reviewing, I did not apply clang-format. I'll do it when the same requirements as the synopsis are true.

1: remove const and ref keyword on value_type
2: better encapsulation (expose only what needs to be)
3: add missing enable_if to free functions
include/sycl_ext_complex.hpp Outdated Show resolved Hide resolved
include/sycl_ext_complex.hpp Show resolved Hide resolved
* implementation: namespace detail becomes namespace cplex::detail
* test: use input instead of std_in for sycl::complex's function
* test: fix typos in the free function's tests
@TApplencourt
Copy link
Collaborator

did I forgot to merge this @jle-quel ? Sorry!

include/sycl_ext_complex.hpp Outdated Show resolved Hide resolved
@bd4
Copy link
Contributor

bd4 commented Jan 16, 2023

I copied the updated header from this PR into gtensor and tried to build the testsuite, and it fails because of the introduction of the extra enable if template argument. Gtensor uses a templated alias type, and I tried applying this difff:

--- a/include/gtensor/complex.h
+++ b/include/gtensor/complex.h
@@ -28,8 +28,8 @@ using complex = thrust::complex<T>;
 #elif defined(GTENSOR_DEVICE_SYCL)
 
 // TODO: this will hopefully be standardized soon and be sycl::complex
-template <typename T>
-using complex = gt::sycl_cplx::complex<T>;
+template <typename T, typename Enable = void>
+using complex = gt::sycl_cplx::complex<T, Enable>;

however it still fails. I don't understand why, but this might be the reason: https://stackoverflow.com/questions/24219357/default-template-arguments-for-the-c11-using-type-alias

The build error I get:

/tmp/icpx-190776/test_expression-header-41ba7c.h:222:487: error: too few template arguments for class template 'complex'
template <> struct KernelInfo<::gt::backend::sycl::AssignN<::gt::gtensor_container<::gt::backend::gtensor_storage<double, ::gt::allocator::caching_allocator<double, ::gt::backend::allocator_impl::wrap_allocator<double, ::gt::backend::allocator_impl::gallocator<::gt::space::sycl>, ::gt::space::sycl>>, ::gt::space::sycl>, 1>, ::gt::gfunction<::gt::funcs::abs, const gt::gfunction<gt::ops::minus, gt::gfunction<gt::funcs::exp, gt::gfunction<gt::ops::multiply, gt::gscalar<gt::sycl_cplx::complex<double> &>, gt::gtensor_container<gt::backend::gtensor_storage<double, gt::allocator::caching_allocator<double, gt::backend::allocator_impl::wrap_allocator<double, gt::backend::allocator_impl::gallocator<gt::space::sycl>, gt::space::sycl> >, gt::space::sycl>, 1> &>, gt::gt_empty_expr>, gt::gtensor_container<gt::backend::gtensor_storage<gt::sycl_cplx::complex<double>, gt::allocator::caching_allocator<gt::sycl_cplx::complex<double>, gt::backend::allocator_impl::wrap_allocator<gt::sycl_cplx::complex<double>, gt::backend::allocator_impl::gallocator<gt::space::sycl>, gt::space::sycl> >, gt::space::sycl>, 1> &> &, ::gt::gt_empty_expr>, ::gt::gtensor_span<double, 1, ::gt::space::sycl>, ::gt::gfunction<::gt::funcs::abs, ::gt::gfunction<::gt::ops::minus, ::gt::gfunction<::gt::funcs::exp, ::gt::gfunction<::gt::ops::multiply, ::gt::gscalar<::gt::sycl_cplx::complex<double, void>>, ::gt::gtensor_span<double, 1, ::gt::space::sycl>>, ::gt::gt_empty_expr>, ::gt::gtensor_span<::gt::sycl_cplx::complex<double, void>, 1, ::gt::space::sycl>>, ::gt::gt_empty_expr>>> {
                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      ^
/tmp/icpx-190776/test_expression-header-41ba7c.h:42:43: note: template is declared here
template <class _Tp, class _Enable> class complex;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~       ^
/tmp/icpx-190776/test_expression-header-41ba7c.h:222:760: error: expected unqualified-id
template <> struct KernelInfo<::gt::backend::sycl::AssignN<::gt::gtensor_container<::gt::backend::gtensor_storage<double, ::gt::allocator::caching_allocator<double, ::gt::backend::allocator_impl::wrap_allocator<double, ::gt::backend::allocator_impl::gallocator<::gt::space::sycl>, ::gt::space::sycl>>, ::gt::space::sycl>, 1>, ::gt::gfunction<::gt::funcs::abs, const gt::gfunction<gt::ops::minus, gt::gfunction<gt::funcs::exp, gt::gfunction<gt::ops::multiply, gt::gscalar<gt::sycl_cplx::complex<double> &>, gt::gtensor_container<gt::backend::gtensor_storage<double, gt::allocator::caching_allocator<double, gt::backend::allocator_impl::wrap_allocator<double, gt::backend::allocator_impl::gallocator<gt::space::sycl>, gt::space::sycl> >, gt::space::sycl>, 1> &>, gt::gt_empty_expr>, gt::gtensor_container<gt::backend::gtensor_storage<gt::sycl_cplx::complex<double>, gt::allocator::caching_allocator<gt::sycl_cplx::complex<double>, gt::backend::allocator_impl::wrap_allocator<gt::sycl_cplx::complex<double>, gt::backend::allocator_impl::gallocator<gt::space::sycl>, gt::space::sycl> >, gt::space::sycl>, 1> &> &, ::gt::gt_empty_expr>, ::gt::gtensor_span<double, 1, ::gt::space::sycl>, ::gt::gfunction<::gt::funcs::abs, ::gt::gfunction<::gt::ops::minus, ::gt::gfunction<::gt::funcs::exp, ::gt::gfunction<::gt::ops::multiply, ::gt::gscalar<::gt::sycl_cplx::complex<double, void>>, ::gt::gtensor_span<double, 1, ::gt::space::sycl>>, ::gt::gt_empty_expr>, ::gt::gtensor_span<::gt::sycl_cplx::complex<double, void>, 1, ::gt::space::sycl>>, ::gt::gt_empty_expr>>> {

Using this instead does not change the error:

 // TODO: this will hopefully be standardized soon and be sycl::complex
 template <typename T>
-using complex = gt::sycl_cplx::complex<T>;
+using complex = gt::sycl_cplx::complex<T, void>;

I appreciate the simplification we get by using some template meta programming (gtensor uses it a lot too), but it unfortunately changes the public interface in ways that make it hard to use. It think type aliasing complex types is a pretty common need for middleware type libraries. Not clear to me what the solution is.

@bd4
Copy link
Contributor

bd4 commented Jan 16, 2023

Note that in that very long error messages, there are instances of both ::gt::sycl_cplx::complex<double, void> and ::gt::sycl_cplx::complex<double>. While it's not clear to me why this is the case, it's also not clear why it would be an issue - that is not a type alias, that is the class, and the default type param should work. The stackoverflow link is unrelated I think.

@bd4
Copy link
Contributor

bd4 commented Jan 16, 2023

Something subtle is going on here. The error does not happen just from using a type alias in a different namespace. It almost seems like an Intel LLVM bug, because it's in a generated file.

@bd4
Copy link
Contributor

bd4 commented Jan 16, 2023

Ok so it's my understanding that the generated file is for the auto-generated kernel name, it's defining a KernelInfo for the Assign operation, and for some reason, it's leaving out a template param for some of the types. It does seem like a bug, but I am having trouble creating a simple reproducer.

@bd4
Copy link
Contributor

bd4 commented Jan 16, 2023

Found a suitable workaround for gtensor that allows me to keep the custom kernel names. I don't think I am motivated to understand why the failure was happening in the first place... in any case, I vote we go ahead and merge this.

@TApplencourt TApplencourt merged commit 0190fa6 into argonne-lcf:main Jan 17, 2023
@TApplencourt
Copy link
Collaborator

Thanks for the review @bd4 !

@jle-quel jle-quel deleted the jle-quel/update-complex-impl-and-tests branch January 17, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants