core/src: Add half math functions to private header

[e10harvey]

Summary of changes

• core/src: Add half math functions to private header

This PR introduces initial support for half math functions which rely on the floating_point_wrapper explicit conversion constructors to cast half_t and bhalf_t arguments to float in order to fall back on the float implementation of math functions. If this fall back approach is found to be a bottleneck by users, we could add a follow on PR to see if invoking backend-specific implementations (where available) of half_t and bhalf_t math functions helps.

Required for kokkos/kokkos-kernels#1774.

[crtrott]

Looks like this needs a rebase? After that I assume its ready for review? We probably should decide which order the two half moving things should come in.

[dalg24]

Please provide a proper description.

Make sure you discuss why we cast to float and do not bother using native half precision intrinsics when available.

[e10harvey]

The CI failures appear to be unrelated to these changes.

[e10harvey]

@dalg24, @crtrott, @lucbv: Ready to merge?

[dalg24]

@dalg24, @crtrott, @lucbv: Ready to merge?

Not reviewed so no not ready to merge. You can try to poll for reviews on nucleus.

[crtrott]

Do we have no tests for this?

[crtrott]

I think we need some nesting. Can we reuse the existing one?

[crtrott]

since we anyway have clang-format off here, can we indent the preprocess if clauses so its easier to see the if/else/endif structure of this nested clause?

[e10harvey]

It is tested already via the float math functions. Shall I add tests for these wrappers?

[e10harvey]

Please provide a proper description.

Make sure you discuss why we cast to float and do not bother using native half precision intrinsics when available.

@dalg24: I made these changes in the description above; does it address your feedback?

[e10harvey]

I think we need some testing. Can we reuse the existing one?

@dalg24: Is it best to add half_t and bhalf_t to the type lists for the existing math function tests?

[masterleinad]

Where are you reenabling/intending to reenable clang-format?
I would appreciate at least some separation after the header include like

Suggested change

	// clang-format off
	#include <Kokkos_MathematicalFunctions.hpp> // For the float overloads
	namespace Kokkos {
	#include <Kokkos_MathematicalFunctions.hpp> // For the float overloads
	// clang-format off
	namespace Kokkos {

[e10harvey]

I think we need some testing. Can we reuse the existing one?

@dalg24: Is it best to add half_t and bhalf_t to the type lists for the existing math function tests?

I do not see a way to reuse the existing one as expected values are computed from std::FUNC.

[dalg24]

A bare minimum test would be calling a few selected math functions on the device and make sure it compiles and executes.
A more rigorous one would probably compare the result to higher precision reference converted to half types.

[masterleinad]

You will need to fix the indentation.

[masterleinad]

Assuming this is passing testing.

[dalg24]

I realize there is virtually no coverage for this with the fundamental types either but where do we define the promotion rules when mixing half types with other fundamental types in binary functions?

[dalg24]

This look fishy

[dalg24]

You would need to provide much more than that.
Where is it being used?

[e10harvey]

The math binary function test calls. Please add what's missing.

[dalg24]

do we mix int and half_t with the first argument as int and not the second?

[e10harvey]

Sorry, I also do not follow this question.

Thank you for your detailed reviews, @dalg24 and @masterleinad.

[dalg24]

I don't quite understand why comparing them as float is the right thing to do.

[e10harvey]

I realize there is virtually no coverage for this with the fundamental types either but where do we define the promotion rules when mixing half types with other fundamental types in binary functions?

@dalg24: I don't understand your question. We always upcast for mixed precision, though. Please feel free to make any further changes you see fit to this PR.

[dalg24]

I realize there is virtually no coverage for this with the fundamental types either but where do we define the promotion rules when mixing half types with other fundamental types in binary functions?

@dalg24: I don't understand your question. We always upcast for mixed precision, though. Please feel free to make any further changes you see fit to this PR.

Is Kokkos::hypot(int, half_t) -> half_t the right return type?
Because we do hypot(int, float) -> double (that is what the standard mandates).
Also Kokkos::hypot(double, half_t) -> ???

[e10harvey]

I realize there is virtually no coverage for this with the fundamental types either but where do we define the promotion rules when mixing half types with other fundamental types in binary functions?

@dalg24: I don't understand your question. We always upcast for mixed precision, though. Please feel free to make any further changes you see fit to this PR.

Is Kokkos::hypot(int, half_t) -> half_t the right return type? Because we do hypot(int, float) -> double (that is what the standard mandates). Also Kokkos::hypot(double, half_t) -> ???

I see. I would expect integral types to be cast to the floating point type, always. I would also expect to always upcast to higher-precision floating point type. In the case of Kokkos::hypot(int, half_t) -> half_t, I think it does make the most sense to cast both arguments to float and return a float.

[e10harvey]

I realize there is virtually no coverage for this with the fundamental types either but where do we define the promotion rules when mixing half types with other fundamental types in binary functions?

@dalg24: I think what you're looking for here are the mixed precision operator overloads defined in Kokkos_Half_FloatingPointWrapper.hpp. IIRC, we do not support mixed precision for operators that result in a binary type.

[dalg24]

I realize there is virtually no coverage for this with the fundamental types either but where do we define the promotion rules when mixing half types with other fundamental types in binary functions?

@dalg24: I don't understand your question. We always upcast for mixed precision, though. Please feel free to make any further changes you see fit to this PR.

Is Kokkos::hypot(int, half_t) -> half_t the right return type? Because we do hypot(int, float) -> double (that is what the standard mandates). Also Kokkos::hypot(double, half_t) -> ???

I see. I would expect integral types to be cast to the floating point type, always. I would also expect to always upcast to higher-precision floating point type. In the case of Kokkos::hypot(int, half_t) -> half_t, I think it does make the most sense to cast both arguments to float and return a float.

No. We want to be aligned with standard C++ which states that integral types will be promoted to double
https://eel.is/c++draft/c.math#cmath.syn-3

[e10harvey]

I don't quite understand why comparing them as float is the right thing to do.

@dalg24: Can you offer a appropriate alternative? e.g. comparing as double.

[dalg24]

Should we be also adding

  long double FUNC(long double x, HALF_TYPE y) {  \
    return Kokkos::FUNC(x, static_cast<long double>(y)); \
  } \
  long double FUNC(HALF_TYPE x, long double y) {  \
    return Kokkos::FUNC(static_cast<long double>(x), y); \
  } \

[e10harvey]

We could do this. It would require adding a cast_from_half specialization that accepts long double as well as a explicit conversion constructor to support static_cast from half to long double. Would you like me to add those changes to this PR?

[e10harvey]

We discussed this in the developer meeting today and came to a consensus. Since long double is not supported in all backend and archuitectures, we will not be adding support for this at this time.

[dalg24]

Just a thought, should we be specializing the function templates rather than adding new overloads?
@nliber please advise

[nliber]

I think overloads are the way to go. I keep going back to the advice in P0551 Thou Shalt Not Specialize std Function Templates! (and his various talks on the subject). Fewer surprises with overloads.

Function template specializations don't participate in overload resolution; basically, they are just an implementation detail once overload resolution picks the function to call.

Changes requested were implemented