SIMD: Split math functions from SIMD_Common.hpp #6487
Conversation
| #include <cstring> | ||
|
|
||
| #include <Kokkos_Core.hpp> | ||
|
|
There was a problem hiding this comment.
does this header not need to explicitly include Kokkos_SIMD_Common.hpp?
There was a problem hiding this comment.
I made it self-contained in 9abaa33.
There was a problem hiding this comment.
Should we check that this is not included without Kokkos_SIMD.hpp being included first?
| @@ -80,7 +80,7 @@ class absolutes { | |||
| template <typename T> | |||
| auto on_host(T const& a) const { | |||
| if constexpr (std::is_signed_v<typename T::value_type>) { | |||
| #if defined(KOKKOS_ENABLE_DEPRECATED_CODE_4) && !defined(KOKKOS_COMPILER_NVCC) | |||
| #if defined(KOKKOS_ENABLE_DEPRECATED_CODE_4) | |||
There was a problem hiding this comment.
What about https://github.com/kokkos/kokkos/pull/6485/files#diff-66339256bc16b14bde61943b2f7e184363640137e9a9b03ccf93c7f1a89e3fa9L98-L102 ?
Should we revert it as well?
There was a problem hiding this comment.
Reverting that change would make us call __host__ functions from a __host__ __device__ function (for host-only types) and we would potentially run into the same error since we would be trying to use vector types in a __host__ __device__ function.
| template <class T, class Abi> | ||
| class simd; | ||
|
|
||
| template <class T, class Abi> | ||
| class simd_mask; | ||
|
|
||
| template <class M, class T> | ||
| class const_where_expression; |
There was a problem hiding this comment.
Why did you go for forward declarations rather than including <Kokkos_SIMD_Common.hpp>?
There was a problem hiding this comment.
To minimize dependencies (even though it probably doesn't matter much here). We should be able to use this header just using the SIMD specializations and not the fallback.
In the end, this pull request shows that the order of overload declarations matters, and including Kokkos_SIMD_Common.hpp might suggest that it would be fine to include that file before the overloads for the simd specializations.
| #include <cstring> | ||
|
|
||
| #include <Kokkos_Core.hpp> | ||
|
|
There was a problem hiding this comment.
Should we check that this is not included without Kokkos_SIMD.hpp being included first?
simd/src/Kokkos_SIMD_Common_Math.hpp
Outdated
| // These are not in the Experimental namespace because their double | ||
| // overloads are not either |
There was a problem hiding this comment.
Is this comment correct? If I'm reading it correctly, it looks like lines 135-143 put the same named function in Experimental (albeit deprecated).
There was a problem hiding this comment.
This pull request just moves code but 229b43d fixes it anyway.
masterleinad
dismissed
nliber’s stale review
The requested changes were addressed.
|
Retest this please. |
|
The CI testing is timing out on an unrelated unit test. |
Follow-up to #6485 (review).
Splitting the math functions from
SIMD_Common.hppand including the new header after including the SIMD types header files fixes the error we were still seeing when usingKokkos::Experimental::abson the host for host-only types withnvcc; with this split,Kokkos::Experimental::absfinds the host-only specialization forKokkos::abs.