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

Half base type #1706

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Half base type #1706

wants to merge 10 commits into from

Conversation

yhmtsai
Copy link
Member

@yhmtsai yhmtsai commented Oct 22, 2024

This pr moves the half out of extended_float.hpp with some modification and half is available in the public interface.

Currently, we can still mark some conversion/operation with GKO_ATTRIBUTE and GKO_INLINE.
However, I think it is good to ensure we only use the vendor's impl on device side.
To achieve it, Jacobi needs to use __half not half now.
This also removes the undefined behavior from reinterpret_cast to std::memcpy

one question: should we mark the half precision in experimental namespace? in usual interface from discussion

@yhmtsai yhmtsai added the 1:ST:ready-for-review This PR is ready for review label Oct 22, 2024
@yhmtsai yhmtsai self-assigned this Oct 22, 2024
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. mod:core This is related to the core module. mod:cuda This is related to the CUDA module. type:preconditioner This is related to the preconditioners mod:hip This is related to the HIP module. labels Oct 22, 2024
@yhmtsai yhmtsai force-pushed the half_type branch 2 times, most recently from 265cb47 to b0c488b Compare October 22, 2024 16:06
@yhmtsai yhmtsai added 1:ST:WIP This PR is a work in progress. Not ready for review. and removed 1:ST:ready-for-review This PR is ready for review labels Oct 22, 2024
@yhmtsai yhmtsai force-pushed the half_type branch 4 times, most recently from 8bd8d1c to 2ab1acd Compare October 23, 2024 12:45
@yhmtsai yhmtsai added 1:ST:ready-for-review This PR is ready for review and removed 1:ST:WIP This PR is a work in progress. Not ready for review. labels Oct 23, 2024
@yhmtsai yhmtsai added this to the Ginkgo 1.9.0 milestone Oct 24, 2024
@MarcelKoch MarcelKoch self-requested a review October 24, 2024 08:14
Copy link
Member

@MarcelKoch MarcelKoch left a comment

Choose a reason for hiding this comment

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

Just to reiterate, I'm in favor of putting this directly into the gko namespace.

include/ginkgo/core/base/half.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/base/half.hpp Show resolved Hide resolved
core/test/base/half.cpp Outdated Show resolved Hide resolved
@yhmtsai
Copy link
Member Author

yhmtsai commented Oct 24, 2024

@MarcelKoch I have made the half without types.hpp dependence such that we can include it directly without circular dependence and the instantiate function have the definition of half now

core/test/base/floating_bit_helper.hpp Outdated Show resolved Hide resolved
core/test/base/floating_bit_helper.hpp Outdated Show resolved Hide resolved
include/ginkgo/core/base/half.hpp Outdated Show resolved Hide resolved
@yhmtsai yhmtsai force-pushed the half_type branch 2 times, most recently from bfbe44b to 0c24e81 Compare October 28, 2024 17:19
Copy link
Member

@thoasm thoasm left a comment

Choose a reason for hiding this comment

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

LGTM, only some nits

reference/test/matrix/coo_kernels.cpp Outdated Show resolved Hide resolved
reference/test/matrix/csr_kernels.cpp Outdated Show resolved Hide resolved
reference/test/matrix/diagonal_kernels.cpp Outdated Show resolved Hide resolved
reference/test/matrix/ell_kernels.cpp Outdated Show resolved Hide resolved
reference/test/matrix/fbcsr_kernels.cpp Outdated Show resolved Hide resolved
reference/test/matrix/hybrid_kernels.cpp Outdated Show resolved Hide resolved
include/ginkgo/core/base/half.hpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-for-review This PR is ready for review mod:core This is related to the core module. mod:cuda This is related to the CUDA module. mod:hip This is related to the HIP module. reg:build This is related to the build system. reg:testing This is related to testing. type:preconditioner This is related to the preconditioners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants