Move half traits to private header and add half/bhalf infinity trait #6055
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dalg24
reviewed
Apr 13, 2023
There was a problem hiding this comment.
Please update the title line of your PR and make sure it mentions this is adding the half precision specializations of the infinity traits that were missing.
This is the most relevant of your PR because that is the only part that is meant to be observable by the user.
e10harvey
changed the title
dalg24
previously approved these changes
Apr 13, 2023
dalg24
reviewed
Apr 13, 2023
| /// bit index: 15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0 | ||
| /// | ||
| template <> | ||
| struct Kokkos::Experimental::Impl::infinity_helper<Kokkos::Experimental::half_t> { |
There was a problem hiding this comment.
you are relying on transitive includes for half_t
There was a problem hiding this comment.
either make the new header self contained or discuss why this is ok here
dalg24
dismissed
their stale review
Withdrawing because new header is not self contained
Comment on lines
20
to
35
| // This header must be included at the bottom of Kokkos_Half.hpp | ||
| // since this is when we know whether half_t and bhalf_t are | ||
| // aliased to float via KOKKOS_HALF_T_IS_FLOAT and | ||
| // KOKKOS_BHALF_T_IS_FLOAT. If half_t and bhalf_t are aliases, | ||
| // we do not want to re-define traits for float. | ||
| // An alternative approach would be to make this header | ||
| // standalone by including Kokkos_Half.hpp here. However, | ||
| // this leads to a circular include if we want to include | ||
| // since they are included in Kokkos_Half.hpp. Nonetheless, | ||
| // we cannot define these traits without first having the | ||
| // user-facing half_t and bhalf_t types defined. Yet another | ||
| // approach would be to make this header user-facing and | ||
| // include them in Kokkos_Core.hpp like we do with | ||
| // Kokkos_NumericTraits.hpp. However, this could lead to | ||
| // usability challenges since half_t and bhalf_t must first | ||
| // be defined. |
There was a problem hiding this comment.
What about moving the content of Kokkos_Half.hpp to an impl header and then only include the impl header and this file in Kokkos_Half.hpp?
e10harvey
changed the title
nliber
approved these changes
May 8, 2023
masterleinad
requested changes
May 9, 2023
e10harvey
changed the title
|
Just posting it here in case it saves some time to other reviewers --- develop
+++ this_pr
@@ -39,17 +39,7 @@
// 2**15 * (1 + 0.9990234375) =
// 65504.0
//
-
-/// \brief: Infinity.
-///
-/// base2 encoding: bits [10,14] set
-/// #define KOKKOS_IMPL_HALF_T_HUGE_VALH 0x7c00
-/// Binary16 encoding:
-/// [s e e e e e f f f f f f f f f f]
-/// [0 1 1 1 1 1 0 0 0 0 0 0 0 0 0 0]
-/// bit index: 15 14 13 12 11 10 9 8 7 6 5 4 3 2 1 0
#if defined(KOKKOS_HALF_T_IS_FLOAT) && !KOKKOS_HALF_T_IS_FLOAT
-
/// \brief: Minimum normalized number
///
/// Stdc defines this as the smallest number (representable in binary16).
@@ -194,7 +184,7 @@
/// \brief: This is the smallest possible exponent value
///
-/// Stdc defines this as the smallest possible exponent value for type binary16.
+/// Stdc defines this as the smallest possible exponent val`ue for type binary16.
/// More precisely, it is the minimum negative integer such that the value min_exponent_helper
/// raised to this power minus 1 can be represented as a normalized floating point number of type float.
///
@@ -309,4 +299,4 @@
static constexpr int value = 128;
};
#endif
-////////////// END BHALF_T (bfloat16) limits //////////////
+////////////// END BHALF_T (bfloat16) limits //////////Please expand the description. Say something like: |
dalg24
reviewed
May 10, 2023
masterleinad
approved these changes
May 10, 2023
Co-authored-by: Damien L-G <dalg24+github@gmail.com>
|
Failures are unrelated (HIP build timing out) |
dalg24
approved these changes
May 14, 2023
Closed
nliber
pushed a commit
to nliber/kokkos
that referenced
this pull request
Jun 22, 2023
…okkos#6055) * core/src: Move half traits to private header * core/src: Add half_t and bhalf_t infinity trait * Update core/src/impl/Kokkos_Half_NumericTraits.hpp Co-authored-by: Damien L-G <dalg24+github@gmail.com> --------- Co-authored-by: Damien L-G <dalg24+github@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
Related to #6045 and kokkos/kokkos-kernels#1774.