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

SIMD: Math functions should be in namespace Kokkos #6465

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

masterleinad
Copy link
Contributor

This fixes the confusion in #6276 (comment).
At the moment, we have fallback SIMD math functions (those that are also defined in cmath) defined in namespace Kokkos but the specialized ones in namespace Kokkos::Experimental.
This pull request makes sure to have all these functions in namespace Kokkos so that users don't have to use different namespaces based on the SIMD type having specializations or not. If DEPRECATED_CODE_4 is enabled, all of these functions are also defined in namespace Kokkos::Experimental similar to what we are doing for the non-SIMD Kokkos math functions.

Copy link
Contributor

@ldh4 ldh4 left a comment

Choose a reason for hiding this comment

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

Conceptually, I agree with this change, but it's unfortunate that files now have to be interleaving with Experimental namespaces multiple times.
Not a suggestion, but it makes me partly wonder if it may be better to aggregate all functions that are no longer in Experimental and place them altogether toward bottom to simplify namespace structure.

template <class T, class Abi> \
[[nodiscard]] KOKKOS_DEPRECATED KOKKOS_FORCEINLINE_FUNCTION simd<T, Abi> \
FUNC(simd<T, Abi> const& a) { \
Kokkos::FUNC(a); \
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like there are couple missing return statements like here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@masterleinad
Copy link
Contributor Author

Not a suggestion, but it makes me partly wonder if it may be better to aggregate all functions that are no longer in Experimental and place them altogether toward bottom to simplify namespace structure.

Yes, I refrained from doing anymore restructuring to avoid conflicts with other pull requests.

@masterleinad
Copy link
Contributor Author

Retest this please.

@masterleinad masterleinad marked this pull request as ready for review September 27, 2023 18:15
@masterleinad
Copy link
Contributor Author

The gcc-8.4.0 CI is timing out and we see the usual OpenMPTarget test failure.

@crtrott crtrott merged commit 3ad6473 into kokkos:develop Oct 4, 2023
27 of 28 checks passed
@crtrott crtrott mentioned this pull request Oct 4, 2023
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

4 participants