Kokkos_ArithTraits: re-implementation using Kokkos Core #1406
Conversation
|
Marking this as WIP until run clang-format on this... |
src/common/Kokkos_ArithTraits.hpp
Outdated
| static KOKKOS_FORCEINLINE_FUNCTION float zero() { return Kokkos::reduction_identity<float>::sum(); } | ||
| static KOKKOS_FORCEINLINE_FUNCTION float one() { return Kokkos::reduction_identity<float>::prod(); } |
There was a problem hiding this comment.
Any good reason why that is not
| static KOKKOS_FORCEINLINE_FUNCTION float zero() { return Kokkos::reduction_identity<float>::sum(); } | |
| static KOKKOS_FORCEINLINE_FUNCTION float one() { return Kokkos::reduction_identity<float>::prod(); } | |
| static KOKKOS_FORCEINLINE_FUNCTION float zero() { return 0; } | |
| static KOKKOS_FORCEINLINE_FUNCTION float one() { return 1; } |
There was a problem hiding this comment.
Not really, it was visually helpful to use the Kokkos function to check that all the calls are using the new implementation instead of the old one. Also if there is ever a problem I get to blame Kokkos Core : )
There was a problem hiding this comment.
But reduction_identity is not part of the numeric traits
There was a problem hiding this comment.
Well there is no namespace, class or struct that defines numeric traits and the reduction_identity are in the numeric traits header... so how would one know?
Is it an issue that I use the definition from the reduction identity?
Otherwise I can always switch to static_cast<floar>(1.0f).
There was a problem hiding this comment.
I meant not part of the facility that was added to replace std::numeric_limits and Kokkos::ArithmeticTraits.
There was a problem hiding this comment.
I updated this to now have all floating point types re-factored.
I just went with static_cast<some_type>(1.0) instead of the reduction_identity which I think is reasonable.
There was a problem hiding this comment.
Thanks, @lucbv. I would advocate for:
- Issuing a warning in our CMake that ArithTraits will be deprecated in 3.7
- Updating KokkosKernels to use the Kokkos math functions directly
- Reverting these changes and decorating all unused functions in ArithTraits with
[[deprecated]]instead - Filing KokkosCore issues for any gaps that should be filled by Kokkos math functions
For example, I can update the CMake. I can also update the KokkosBatched namespace / unit tests. Perhaps someone can do the same for sparse and another person for blas.
Once this is done, we can make another pass and decorate ArithTraits functions with [[deprecated]] and file an issue in KokkosCore.
Thoughts?
|
@e10harvey |
lucbv
force-pushed
the
ArithTraits_update
branch
2 times, most recently
from
0f59b69 to
4545cfb
Compare
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9_GCC720_Light_Tpls_GCC720_GCC740
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10_Tpls_CUDA10_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_INTEL18
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG1001
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
This change should not affect users directly as it is only
an implementation change. Using the Kokkos math functions
and numeric traits, the arithmetic traits are implemented
in a more portable way.
Use `digits` for `t` implementation
Use `finite_{min,max}` to implement `{min,max}`
Applying clang-format
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9_GCC720_Light_Tpls_GCC720_GCC740
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10_Tpls_CUDA10_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_INTEL18
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG1001
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
@lucbv: We have ArithTraits overloads for We are tracking half precision scalar support via kokkos/kokkos#3531 and HIP supports We have not discussed adding C++ std mathematical library functions for half precision scalars yet; arith traits was updated to support the BatchedGemm work. It would be good to add math function support for half in Kokkos Core; I will propose this during the developer meeting next week. |
There was a problem hiding this comment.
Thank you, @lucbv. This refactor is really helpful. If we're going to keep Arith Traits around, now would be a good time to reduce our maintenance burden further. From what I can see, we do not need separate specializations for each floating point type. For half_t and bhalf_t we may need two non-conditional specializations as they currently are -- note that you can use static_cast on half_t and bhalf_t and we may reduce the code duplication further by defining a castType in Arith Traits. For float, double, long double, and perhaps integral types, I believe we can simply use std::enable_if_t. Christian showed me how to do this when we added half precision support. For example, see https://github.com/kokkos/kokkos/blob/master/core/src/Kokkos_Half.hpp#L549. If we're keeping Arith Traits around, let's reduce duplicate code here fruther by specializing via:
template <class T>
std::enable_if_t<
std::is_same<T, float>::value || std::is_same<T, double>::value, T || std::is_same<T, long double>::value || ...>
class ArithTraits<T> ...
src/common/Kokkos_ArithTraits.hpp
Outdated
| static KOKKOS_FORCEINLINE_FUNCTION mag_type rmin() { | ||
| return FLT_MIN; // ??? // should be base^(emin-1) | ||
| return Kokkos::Experimental::norm_min<val_type>::value; // ??? // should be |
There was a problem hiding this comment.
| return Kokkos::Experimental::norm_min<val_type>::value; // ??? // should be | |
| return Kokkos::Experimental::norm_min<val_type>::value; |
src/common/Kokkos_ArithTraits.hpp
Outdated
| } | ||
| static KOKKOS_FORCEINLINE_FUNCTION mag_type eps() { return epsilon(); } | ||
| static KOKKOS_FORCEINLINE_FUNCTION mag_type sfmin() { | ||
| return FLT_MIN; // ??? | ||
| return Kokkos::Experimental::norm_min<val_type>::value; // ??? |
There was a problem hiding this comment.
| return Kokkos::Experimental::norm_min<val_type>::value; // ??? | |
| return Kokkos::Experimental::norm_min<val_type>::value; |
| #include <Kokkos_Macros.hpp> | ||
| #include <KokkosKernels_Half.hpp> |
There was a problem hiding this comment.
I am okay to move KokkosKernels_Half.hpp to Kokkos Core as we already have most of the half_t and bhalf_t implementation there.
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9_GCC720_Light_Tpls_GCC720_GCC740
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10_Tpls_CUDA10_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_INTEL18
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG1001
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
@e10harvey |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9_GCC720_Light_Tpls_GCC720_GCC740
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10_Tpls_CUDA10_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_INTEL18
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG1001
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9_GCC720_Light_Tpls_GCC720_GCC740
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10_Tpls_CUDA10_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_INTEL18
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG1001
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
@e10harvey @dalg24 |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
src/common/Kokkos_ArithTraits.hpp
Outdated
| static FUNC_QUAL val_type zero() { return static_cast<val_type>(0.0); } \ | ||
| static FUNC_QUAL val_type one() { return static_cast<val_type>(1.0); } \ |
There was a problem hiding this comment.
| static FUNC_QUAL val_type zero() { return static_cast<val_type>(0.0); } \ | |
| static FUNC_QUAL val_type one() { return static_cast<val_type>(1.0); } \ | |
| static FUNC_QUAL val_type zero() { return static_cast<val_type>(0); } \ | |
| static FUNC_QUAL val_type one() { return static_cast<val_type>(1); } \ |
src/common/Kokkos_ArithTraits.hpp
Outdated
| static FUNC_QUAL mag_type real(const val_type x) { return x; } \ | ||
| static FUNC_QUAL mag_type imag(const val_type) { return zero(); } \ | ||
| static FUNC_QUAL val_type conj(const val_type x) { return x; } \ |
There was a problem hiding this comment.
I would have done
| static FUNC_QUAL mag_type real(const val_type x) { return x; } \ | |
| static FUNC_QUAL mag_type imag(const val_type) { return zero(); } \ | |
| static FUNC_QUAL val_type conj(const val_type x) { return x; } \ | |
| static FUNC_QUAL mag_type real(const val_type x) { return Kokkos::real(x); } \ | |
| static FUNC_QUAL mag_type imag(const val_type x) { return Kokkos::imag(x); } \ | |
| static FUNC_QUAL val_type conj(const val_type x) { return Kokkos::conj(x); } \ |
There was a problem hiding this comment.
Actually since Kokkos::conj() returns a complex number it will not be possible to use it in this context.
src/common/Kokkos_ArithTraits.hpp
Outdated
| static KOKKOS_FUNCTION mag_type real(const val_type x) { return x; } \ | ||
| static KOKKOS_FUNCTION mag_type imag(const val_type) { return zero(); } \ | ||
| static KOKKOS_FUNCTION val_type conj(const val_type x) { return x; } \ |
There was a problem hiding this comment.
| static KOKKOS_FUNCTION mag_type real(const val_type x) { return x; } \ | |
| static KOKKOS_FUNCTION mag_type imag(const val_type) { return zero(); } \ | |
| static KOKKOS_FUNCTION val_type conj(const val_type x) { return x; } \ | |
| static KOKKOS_FUNCTION mag_type real(const val_type x) { return Kokkos::real(x); } \ | |
| static KOKKOS_FUNCTION mag_type imag(const val_type x) { return Kokkos::imag(x); } \ | |
| static KOKKOS_FUNCTION val_type conj(const val_type x) { return Kokkos::conj(x); } \ |
|
|
||
| #define KOKKOSKERNELS_SIGNED_ABS \ | ||
| static KOKKOS_FUNCTION mag_type abs(const val_type x) { \ | ||
| return Kokkos::abs(x); \ |
There was a problem hiding this comment.
Just making sure you understand this will involve promotion to int for the smaller integer types.
If you are using it for signed integers you might as well used it for the unsigned ones.
| static KOKKOS_FUNCTION val_type sqrt(const val_type x) { \ | ||
| return static_cast<val_type>(Kokkos::sqrt(abs(x))); \ | ||
| } \ | ||
| static KOKKOS_FUNCTION val_type cbrt(const val_type x) { \ | ||
| return static_cast<val_type>(Kokkos::cbrt(abs(x))); \ | ||
| } \ | ||
| static KOKKOS_FUNCTION val_type exp(const val_type x) { \ | ||
| return static_cast<val_type>(Kokkos::exp(abs(x))); \ | ||
| } \ | ||
| static KOKKOS_FUNCTION val_type log(const val_type x) { \ | ||
| return static_cast<val_type>(Kokkos::log(abs(x))); \ | ||
| } \ | ||
| static KOKKOS_FUNCTION val_type log10(const val_type x) { \ | ||
| return static_cast<val_type>(Kokkos::log10(abs(x))); \ | ||
| } \ |
There was a problem hiding this comment.
Why are we taking the absolute value?!! (I did see that this is how it currently is implemented)
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
3 similar comments
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
@dalg24 @e10harvey |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9_GCC720_Light_Tpls_GCC720_GCC740
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10_Tpls_CUDA10_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_INTEL18
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG1001
Jenkins Parameters
Using Repos:
Pull Request Author: lucbv |
Using macro to implement __float128 after Kokkos PR #5081 merged. Also improving macros for complex and integral types, making these almost completely auto-generated by the macro with the exception of a few definitions and the name() method.
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA9_GCC720_Light_Tpls_GCC720_GCC740
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_CUDA10_Tpls_CUDA10_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_GCC720_Light_LayoutRight
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_GCC720
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_Tpls_INTEL18
Jenkins Parameters
Build InformationTest Name: KokkosKernels_PullRequest_CLANG1001
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ dalg24 ]! |
|
Status Flag 'Pull Request AutoTester' - Pull Request MUST BE MERGED MANUALLY BY Project Team - This Repo does not support Automerge |
| #include <Kokkos_Macros.hpp> | ||
| #include <KokkosKernels_Half.hpp> |
This change should not affect users directly as it is only an implementation change. Using the Kokkos math functions and numeric traits, the arithmetic traits are implemented in a more portable way.
At this point only the
floattype the implementation has been updated to gather feedback before the work continues to involved all the other supported types.In the short term this also helps with SYCL development which is a bit tricky...