Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add bit_cast function template to <Kokkos_BitManipulation.hpp> #6101

Merged
merged 8 commits into from
May 5, 2023

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented May 4, 2023

This was the last function template missing that is present in the header <bit> from the standard library.
One thing worth noting is that Kokkos::bit_cast is not usable in constant expressions. It is not implementable as a library and needs compiler magic to work so we just can't declare it constexpr.

I went ahead and replaced the Kokkos::Experimental::bit_cast in the SIMD sub package. We had agreed we'd do so when it is provided by Core.

…class declaration within the body of the functor)
Comment on lines -29 to -35
template <class To, class From>
[[nodiscard]] KOKKOS_FORCEINLINE_FUNCTION constexpr To bit_cast(
From const& src) {
To dst;
std::memcpy(&dst, &src, sizeof(To));
return dst;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ldh4 @ibaned I have not tested and I suspect it might not actually be covered by the CI

Copy link
Contributor

@ldh4 ldh4 May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's used in simd constructors.

template <class U, std::enable_if_t<std::is_convertible_v<U, value_type>,
bool> = false>
KOKKOS_IMPL_HOST_FORCEINLINE_FUNCTION simd(U&& value)
: m_value(_mm256_set1_epi32(bit_cast<std::int32_t>(value_type(value)))) {}

I'm not sure if I've seen it used in the test.

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming the suggested fixes are applied.

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about my suggestion of using the memcmp in the check function?

template <typename To, typename From>
static KOKKOS_FUNCTION bool check(const From& from) {
using Kokkos::Experimental::bit_cast_builtin;
return bit_cast_builtin<From>(bit_cast_builtin<To>(from)) == from;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use my_memcmp here and pass both the from and to value in. Otherwise you could have something like bit_cast_builtin calls reverse order of bits and you still would pass?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to have a few round trips checks and there is "coverage" for my_memcmp below in check 6 and 7 below

Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok fine with me

Co-authored-by: Daniel Arndt <arndtd@ornl.gov>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants