Update common mathematical functions #4043

dalg24 · 2021-05-22T20:16:38Z

Refactor definition of common math functions (attempt to reduce complexity and improve readability)
Add missing *f and *l "overloads" for float and long double
Implement function that return quiet NaN for SYCL

crtrott · 2021-06-04T14:55:07Z

Whats the status of this?

dalg24 · 2021-06-04T16:20:59Z

Whats the status of this?

Ready for review

dalg24 · 2021-06-04T16:23:56Z

@lucbv please review

@masterleinad or @nliber please review the adding a definition of NaN for SYCL

masterleinad · 2021-06-04T18:29:33Z

core/src/Kokkos_MathematicalFunctions.hpp

-  KOKKOS_INLINE_FUNCTION RETURNTYPE FUNC(ARGTYPE x) {                        \
-    using NAMESPACE_MATH_FUNCTIONS::FUNC;                                    \
-    return FUNC(x);                                                          \
+#if defined(KOKKOS_IMPL_MATH_FUNCTIONS_HAVE_LONG_DOUBLE_OVERLOADS)


Why are there overloads other than for long double guarded here? I would prefer to avoid the duplicated code if possible.

What alternative do you suggest?

Something like

diff --git a/core/src/Kokkos_MathematicalFunctions.hpp b/core/src/Kokkos_MathematicalFunctions.hpp index 50fde82d7..8c378559b 100644 --- a/core/src/Kokkos_MathematicalFunctions.hpp +++ b/core/src/Kokkos_MathematicalFunctions.hpp @@ -100,9 +100,7 @@ namespace Experimental { #define KOKKOS_IMPL_MATH_FUNCTIONS_HAVE_LONG_DOUBLE_OVERLOADS #endif -#if defined(KOKKOS_IMPL_MATH_FUNCTIONS_HAVE_LONG_DOUBLE_OVERLOADS) - -#define KOKKOS_IMPL_MATH_UNARY_FUNCTION(FUNC) \ +#define KOKKOS_IMPL_MATH_UNARY_FUNCTION_OTHER(FUNC) \ KOKKOS_INLINE_FUNCTION float FUNC(float x) { \ using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ return FUNC(x); \ @@ -111,18 +109,10 @@ namespace Experimental { using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ return FUNC(x); \ } \ - KOKKOS_INLINE_FUNCTION long double FUNC(long double x) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(x); \ - } \ KOKKOS_INLINE_FUNCTION float FUNC##f(float x) { \ using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ return FUNC(x); \ } \ - KOKKOS_INLINE_FUNCTION long double FUNC##l(long double x) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(x); \ - } \ template <class T> \ KOKKOS_INLINE_FUNCTION std::enable_if_t<std::is_integral<T>::value, double> \ FUNC(T x) { \ @@ -130,7 +120,23 @@ namespace Experimental { return FUNC(static_cast<double>(x)); \ } -#define KOKKOS_IMPL_MATH_UNARY_PREDICATE(FUNC) \ +#if defined(KOKKOS_IMPL_MATH_FUNCTIONS_HAVE_LONG_DOUBLE_OVERLOADS) +#define KOKKOS_IMPL_MATH_UNARY_FUNCTION(FUNC) \ + KOKKOS_IMPL_MATH_UNARY_FUNCTION_OTHER(FUNC) \ + KOKKOS_INLINE_FUNCTION long double FUNC(long double x) { \ + using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ + return FUNC(x); \ + } \ + KOKKOS_INLINE_FUNCTION long double FUNC##l(long double x) { \ + using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ + return FUNC(x); \ + } \ +#else +#define KOKKOS_IMPL_MATH_UNARY_FUNCTION(FUNC) \ + KOKKOS_IMPL_MATH_UNARY_FUNCTION_OTHER(FUNC) +#endif + +#define KOKKOS_IMPL_MATH_UNARY_PREDICATE_OTHER(FUNC) \ KOKKOS_INLINE_FUNCTION bool FUNC(float x) { \ using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ return FUNC(x); \ @@ -139,10 +145,6 @@ namespace Experimental { using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ return FUNC(x); \ } \ - KOKKOS_INLINE_FUNCTION bool FUNC(long double x) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(x); \ - } \ template <class T> \ KOKKOS_INLINE_FUNCTION std::enable_if_t<std::is_integral<T>::value, bool> \ FUNC(T x) { \ @@ -150,7 +152,19 @@ namespace Experimental { return FUNC(static_cast<double>(x)); \ } -#define KOKKOS_IMPL_MATH_BINARY_FUNCTION(FUNC) \ +#if defined(KOKKOS_IMPL_MATH_FUNCTIONS_HAVE_LONG_DOUBLE_OVERLOADS) +#define KOKKOS_IMPL_MATH_UNARY_PREDICATE(FUNC) \ + KOKKOS_IMPL_MATH_UNARY_PREDICATE_OTHER(FUNC) \ + KOKKOS_INLINE_FUNCTION bool FUNC(long double x) { \ + using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ + return FUNC(x); \ + } +#else +#define KOKKOS_IMPL_MATH_UNARY_PREDICATE(FUNC) \ + KOKKOS_IMPL_MATH_UNARY_PREDICATE_OTHER(FUNC) +#endif + +#define KOKKOS_IMPL_MATH_BINARY_FUNCTION_OTHER(FUNC) \ KOKKOS_INLINE_FUNCTION float FUNC(float x, float y) { \ using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ return FUNC(x, y); \ @@ -181,57 +195,29 @@ namespace Experimental { return FUNC(static_cast<Promoted>(x), static_cast<Promoted>(y)); \ } -#else // long double overloads are not available - -#define KOKKOS_IMPL_MATH_UNARY_FUNCTION(FUNC) \ - KOKKOS_INLINE_FUNCTION float FUNC(float x) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(x); \ - } \ - KOKKOS_INLINE_FUNCTION double FUNC(double x) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(x); \ - } \ - KOKKOS_INLINE_FUNCTION float FUNC##f(float x) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(x); \ - } \ - template <class T> \ - KOKKOS_INLINE_FUNCTION std::enable_if_t<std::is_integral<T>::value, double> \ - FUNC(T x) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(static_cast<double>(x)); \ - } - -#define KOKKOS_IMPL_MATH_UNARY_PREDICATE(FUNC) \ - KOKKOS_INLINE_FUNCTION bool FUNC(float x) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(x); \ - } \ - KOKKOS_INLINE_FUNCTION bool FUNC(double x) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(x); \ - } \ - template <class T> \ - KOKKOS_INLINE_FUNCTION std::enable_if_t<std::is_integral<T>::value, bool> \ - FUNC(T x) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(static_cast<double>(x)); \ +#if defined(KOKKOS_IMPL_MATH_FUNCTIONS_HAVE_LONG_DOUBLE_OVERLOADS) +#define KOKKOS_IMPL_MATH_BINARY_FUNCTION(FUNC) \ + KOKKOS_IMPL_MATH_BINARY_FUNCTION_OTHER(FUNC) \ + KOKKOS_INLINE_FUNCTION long double FUNC(long double x, long double y) { \ + using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ + return FUNC(x, y); \ + } \ + KOKKOS_INLINE_FUNCTION long double FUNC##l(long double x, long double y) { \ + using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ + return FUNC(x, y); \ + } \ + template <class T1, class T2> \ + KOKKOS_INLINE_FUNCTION std::enable_if_t<std::is_arithmetic<T1>::value && \ + std::is_arithmetic<T2>::value, \ + Kokkos::Impl::promote_2_t<T1, T2>> \ + FUNC(T1 x, T2 y) { \ + using Promoted = Kokkos::Impl::promote_2_t<T1, T2>; \ + using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ + return FUNC(static_cast<Promoted>(x), static_cast<Promoted>(y)); \ } - +#else #define KOKKOS_IMPL_MATH_BINARY_FUNCTION(FUNC) \ - KOKKOS_INLINE_FUNCTION float FUNC(float x, float y) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(x, y); \ - } \ - KOKKOS_INLINE_FUNCTION double FUNC(double x, double y) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(x, y); \ - } \ - KOKKOS_INLINE_FUNCTION float FUNC##f(float x, float y) { \ - using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ - return FUNC(x, y); \ - } \ + KOKKOS_IMPL_MATH_BINARY_FUNCTION_OTHER(FUNC) \ template <class T1, class T2> \ KOKKOS_INLINE_FUNCTION std::enable_if_t< \ std::is_arithmetic<T1>::value && std::is_arithmetic<T2>::value && \ @@ -243,7 +229,6 @@ namespace Experimental { using KOKKOS_IMPL_MATH_FUNCTIONS_NAMESPACE::FUNC; \ return FUNC(static_cast<Promoted>(x), static_cast<Promoted>(y)); \ } - #endif // Basic operations

optionally combing all the branches.

Nah I don't like it

What's your argument for having the duplications then?

It's what I was trying to move away from with the refactor. The goal was less macro to reduce complexity and increase readability, even if it means duplicating code.

I guess we disagree on this point here then. 🙂

core/src/Kokkos_MathematicalFunctions.hpp

dalg24 · 2021-06-04T20:57:36Z

Retest this please

lucbv

Thanks for the changes @dalg24 this looks good

dalg24 force-pushed the fixup_common_math_functions branch 2 times, most recently from ea15a29 to 5435c6a Compare May 24, 2021 13:36

dalg24 marked this pull request as ready for review May 24, 2021 13:39

dalg24 marked this pull request as draft May 24, 2021 15:53

dalg24 added 5 commits June 4, 2021 11:55

Move integer promotion traits to use with math functions

e68e2b3

Refactor math function to avoid accidental use of double precision

5e088d1

Add missing suffixed math functions overloads for float and long double

c9cb8b7

Add missing quiet NaN math function for SYCL (ignores nancode)

edcd8ed

Move definition of absolute value dunction to the right place

dc2933d

dalg24 force-pushed the fixup_common_math_functions branch from 5435c6a to dc2933d Compare June 4, 2021 16:19

dalg24 changed the title ~~Fix defect in common mathematical functions~~ Update common mathematical functions Jun 4, 2021

dalg24 marked this pull request as ready for review June 4, 2021 16:20

dalg24 force-pushed the fixup_common_math_functions branch 3 times, most recently from 50d39d0 to 56d69f0 Compare June 4, 2021 17:45

nliber approved these changes Jun 4, 2021

View reviewed changes

Update absolute value function definition

c184bb4

dalg24 force-pushed the fixup_common_math_functions branch from 56d69f0 to c184bb4 Compare June 4, 2021 18:28

masterleinad reviewed Jun 4, 2021

View reviewed changes

dalg24 added 3 commits June 4, 2021 14:59

Attempt to unify absolute value implementation

fe1737c

Comment about the current SYCL quiet NaN implementation

d19d386

Comment about nearbyint not being available with SYCL

1bcd28f

lucbv approved these changes Jun 9, 2021

View reviewed changes

crtrott merged commit 7695b81 into kokkos:develop Jun 9, 2021

dalg24 deleted the fixup_common_math_functions branch June 9, 2021 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update common mathematical functions #4043

Update common mathematical functions #4043

dalg24 commented May 22, 2021 •

edited

crtrott commented Jun 4, 2021

dalg24 commented Jun 4, 2021

dalg24 commented Jun 4, 2021

masterleinad Jun 4, 2021

dalg24 Jun 4, 2021

masterleinad Jun 4, 2021

dalg24 Jun 4, 2021

masterleinad Jun 4, 2021

dalg24 Jun 4, 2021

masterleinad Jun 4, 2021

dalg24 commented Jun 4, 2021

lucbv left a comment

Update common mathematical functions #4043

Update common mathematical functions #4043

Conversation

dalg24 commented May 22, 2021 • edited

crtrott commented Jun 4, 2021

dalg24 commented Jun 4, 2021

dalg24 commented Jun 4, 2021

masterleinad Jun 4, 2021

Choose a reason for hiding this comment

dalg24 Jun 4, 2021

Choose a reason for hiding this comment

masterleinad Jun 4, 2021

Choose a reason for hiding this comment

dalg24 Jun 4, 2021

Choose a reason for hiding this comment

masterleinad Jun 4, 2021

Choose a reason for hiding this comment

dalg24 Jun 4, 2021

Choose a reason for hiding this comment

masterleinad Jun 4, 2021

Choose a reason for hiding this comment

dalg24 commented Jun 4, 2021

lucbv left a comment

Choose a reason for hiding this comment

dalg24 commented May 22, 2021 •

edited