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

Ensure that Kokkos::complex only gets instantiated for cv-unqualified floating-point types #6251

Merged

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Jul 1, 2023

#6069 (comment) uncovered a bug where we accidentally instantiated Kokkos::complex for integral types.

This PR is open as a base for discussion and proposes to static assert that complex is being instantiated for a cv-unqualified floating-point type. The current implementation would not allow for using half types.

@dalg24 dalg24 requested review from nliber and e10harvey July 1, 2023 20:46
@dalg24 dalg24 marked this pull request as ready for review July 5, 2023 18:48
@dalg24
Copy link
Member Author

dalg24 commented Jul 5, 2023

Do you guys want a comment about half/bhalf type support?

@@ -44,6 +44,11 @@ class
alignas(2 * sizeof(RealType))
#endif
complex {
static_assert(std::is_floating_point_v<RealType> &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we merge this and then add a follow on PR with:

Suggested change
static_assert(std::is_floating_point_v<RealType> &&
static_assert((std::is_floating_point_v<RealType> &&
std::is_same_v<RealType, std::remove_cv_t<RealType>) ||
kokkos_type_is_half<RealType>::value)

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see any usage of Kokkos::complex with half_t or bhalf_t in Trilinos:
https://github.com/search?q=repo%3Atrilinos%2FTrilinos+Kokkos%3A%3Acomplex+half_t&type=code

Copy link
Member Author

Choose a reason for hiding this comment

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

Except that

  • kokkos_type_is_half is not yet a thing
  • that including <Kokkos_Half.hpp> is not acceptable

so we would have to explore some other mechanism to enable if we decide to add support.

@dalg24
Copy link
Member Author

dalg24 commented Jul 5, 2023

Ignoring timeouts in OpenMPTarget build

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.

None yet

5 participants