implement conversion between half and single precision for denormals by emre-safa · Pull Request #1927 · ginkgo-project/ginkgo

emre-safa · 2025-09-03T09:43:23Z

conversion from single to half will be provided

yhmtsai

Nice work!
I will suggest you put comments to describe leading_zeros, new_exponent, new_significand operation.

core/test/base/half.cpp

yhmtsai · 2025-09-03T09:54:10Z

include/ginkgo/core/base/half.hpp

+            }
+
+            auto leading_zeros =
+                __builtin_clz(f16_traits::significand_mask & data_) -


Suggested change

__builtin_clz(f16_traits::significand_mask & data_) -

__builtin_clz(static_cast<std::uint32_t>(f16_traits::significand_mask & data_)) -

Because __builtin_clz only support unsigned integer, making it more explicitly will help understanding.

Additionally you will need to create a clz function in detail namespace to make it also support MSVC platform.
Check bitscanreverse and core/base/intrinsics.hpp
https://learn.microsoft.com/en-us/cpp/intrinsics/bitscanreverse-bitscanreverse64?view=msvc-170

yhmtsai · 2025-09-03T10:01:36Z

include/ginkgo/core/base/half.hpp

+            auto new_exponent =
+                ((conv::bias_change >> f32_traits::significand_bits) -
+                 leading_zeros)
+                << f32_traits::significand_bits;


Suggested change

auto new_exponent =

((conv::bias_change >> f32_traits::significand_bits) -

leading_zeros)

<< f32_traits::significand_bits;

auto new_exponent =

conv::bias_change - (leading_zeros << f32_traits::significand_bits);

It also worth noting leading_zeros is alway less than bias change because we are at half -> float

…added

core/test/base/half.cpp

include/ginkgo/core/base/half.hpp

…ions

yhmtsai

LGTM!
some improvement and the test suggestion

include/ginkgo/core/base/half.hpp

yhmtsai · 2025-09-12T13:42:45Z

include/ginkgo/core/base/half.hpp

+                     f16_traits::significand_bits) +
+                    f32_traits::significand_bits -
+                    f16_traits::significand_bits + 1;
+                if (tail_length > f32_traits::significand_bits + 1) {


Suggested change

if (tail_length > f32_traits::significand_bits + 1) {

// all significant (including implicitly leading 1) will be moved after representation field more than one digit (less than half) such that it will rounding to zero.

if (tail_length > f32_traits::significand_bits + 1) {

please feel free to rephrase the sentence

include/ginkgo/core/base/half.hpp

core/test/base/half.cpp

yhmtsai

LGTM. some corner test are required

yhmtsai · 2025-09-23T10:14:14Z

include/ginkgo/core/base/half.hpp

+                    (conv::bias_change - ((data_ & f32_traits::exponent_mask) >>
+                                          conv::significand_offset) >>
+                     f16_traits::significand_bits) +


Suggested change

(conv::bias_change - ((data_ & f32_traits::exponent_mask) >>

conv::significand_offset) >>

f16_traits::significand_bits) +

((conv::bias_change - ((data_ & f32_traits::exponent_mask) >>

conv::significand_offset)) >>

f16_traits::significand_bits) +

to ensure the ordering and avoid warning

yhmtsai · 2025-09-23T10:16:09Z

include/ginkgo/core/base/half.hpp

+                __builtin_clz(static_cast<std::uint32_t>(
+                    f16_traits::significand_mask & data_)) -
+                f16_traits::exponent_bits - f16_traits::sign_bits -
+                8 * (sizeof(conv::result_bits) - sizeof(conv::source_bits));


Suggested change

8 * (sizeof(conv::result_bits) - sizeof(conv::source_bits));

CHAR_BIT * (sizeof(conv::result_bits) - sizeof(conv::source_bits));

this is predefined macro from climits

yhmtsai · 2025-09-23T12:08:54Z

core/test/base/half.cpp

+}
+
+
+TEST(FloatToHalf, RoundsUpToEvenNumber)


maybe add another test from rounds down to even number by adding one last bit into the end to ensure we compare full tail before shift.

yhmtsai · 2025-10-30T09:30:23Z

Hi @emre-safa , do you have some time to work on this?

emre-safa · 2025-11-05T10:05:41Z

Hi @emre-safa , do you have some time to work on this?

I thought this job was done, I will have a look at it as soon as I have time. Sorry for late reply.

ginkgo-bot · 2025-11-10T14:54:16Z

Error: The following files need to be formatted:

benchmark/matrix_generator/matrix_generator.cpp
benchmark/utils/runner.hpp
cuda/base/exception.cpp
dpcpp/factorization/par_ilut_select_kernel.dp.cpp
examples/performance-debugging/performance-debugging.cpp
include/ginkgo/core/stop/iteration.hpp
include/ginkgo/core/stop/residual_norm.hpp
include/ginkgo/extensions/kokkos/spaces.hpp
test/matrix/hybrid_kernels.cpp

You can find a formatting patch under Artifacts here or run format! if you have write access to Ginkgo

implement half to single conversion for denormals

709d320

emre-safa requested a review from yhmtsai September 3, 2025 09:43

emre-safa self-assigned this Sep 3, 2025

emre-safa added the 1:ST:ready-for-review This PR is ready for review label Sep 3, 2025

ginkgo-bot added reg:testing This is related to testing. mod:core This is related to the core module. labels Sep 3, 2025

yhmtsai requested changes Sep 3, 2025

View reviewed changes

emre-safa added 4 commits September 4, 2025 11:28

add comments for variables and a new test

39e76b1

Implement float to half conversion for denormals. More tests will be …

e43238b

…added

add more tests for float to half

05f7e86

make half to single conversion support MSVC platform

3af5540

yhmtsai reviewed Sep 12, 2025

View reviewed changes

core/test/base/half.cpp Show resolved Hide resolved

yhmtsai requested changes Sep 12, 2025

View reviewed changes

include/ginkgo/core/base/half.hpp Outdated Show resolved Hide resolved

Make float-to-half conversion portable across all downcasting convers…

fbc7b3b

…ions

yhmtsai requested changes Sep 12, 2025

View reviewed changes

emre-safa added 2 commits September 12, 2025 17:09

some improvement and add a new test

312ef7e

add a comment line

fc1ad57

yhmtsai requested changes Sep 23, 2025

View reviewed changes

minor improvements and additional test

1bcfb05

yhmtsai added this to the Ginkgo 1.11 milestone Nov 13, 2025

	__builtin_clz(f16_traits::significand_mask & data_) -
	__builtin_clz(static_cast<std::uint32_t>(f16_traits::significand_mask & data_)) -

	if (tail_length > f32_traits::significand_bits + 1) {
	// all significant (including implicitly leading 1) will be moved after representation field more than one digit (less than half) such that it will rounding to zero.
	if (tail_length > f32_traits::significand_bits + 1) {

	8 * (sizeof(conv::result_bits) - sizeof(conv::source_bits));
	CHAR_BIT * (sizeof(conv::result_bits) - sizeof(conv::source_bits));

		}


		TEST(FloatToHalf, RoundsUpToEvenNumber)

Conversation

emre-safa commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yhmtsai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yhmtsai Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

yhmtsai Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

yhmtsai Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yhmtsai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yhmtsai Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yhmtsai left a comment

Choose a reason for hiding this comment

Uh oh!

yhmtsai Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

yhmtsai Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

yhmtsai Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

yhmtsai commented Oct 30, 2025

Uh oh!

emre-safa commented Nov 5, 2025

Uh oh!

ginkgo-bot commented Nov 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

emre-safa commented Sep 3, 2025 •

edited

Loading