[libclc] Improve nextafter behaviour around zero #127469
Merged
Conversation
This file contains hidden or 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
|
With this PR and with #126905 I achieved a fully passing OpenCL-CTS conversions run. |
arsenm
reviewed
Feb 18, 2025
This commit improves the behaviour of (__clc_)nextafter around zero. Specifically, the nextafter value of very small negative numbers in the positive direction is now negative zero. Previously we'd return positive zero. This behaviour is not required as far as OpenCL is concerned: at least, the CTS isn't testing for it. However, this change does bring our implementation into bit-equivalence with (libstdc++'s implementation of) std::nextafter, tested on all possible values of 32-bit float towards both positive and negative INFINITY. Furthermore, since the implementation of libclc's floating-point 'rtp' and 'rtz' conversions use __clc_nextafter, the previous behaviour was resulting in CTS validation issues. For example, when converting float -0x1.000002p-25 to half, rounding towards zero or positive infinity, nextafter was returning +0.0, whereas the correct conversion requires us to return -0.0. We could work around this issue in the conversion functions, but since the change to nextafter is small enough and the behaviour around zero matches libstdc++, the fix feels at home there. This commit also converts several variables to unsigned types to avoid undefined behaviour surrounding signed underflow on the subtractions.
frasercrmck
force-pushed
the
libclc-clc-nextafter-zeroes
branch
from
ae75e98 to
e25ac41
Compare
arsenm
approved these changes
Feb 19, 2025
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.
This commit improves the behaviour of (_clc)nextafter around zero. Specifically, the nextafter value of very small negative numbers in the positive direction is now negative zero. Previously we'd return positive zero.
This behaviour is not required as far as OpenCL is concerned: at least, the CTS isn't testing for it. However, this change does bring our implementation into bit-equivalence with (libstdc++'s implementation of) std::nextafter, tested on all possible values of 32-bit float towards both positive and negative INFINITY.
Furthermore, since the implementation of libclc's floating-point 'rtp' and 'rtz' conversions use __clc_nextafter, the previous behaviour was resulting in CTS validation issues. For example, when converting float -0x1.000002p-25 to half, rounding towards zero or positive infinity, nextafter was returning +0.0, whereas the correct conversion requires us to return -0.0.
We could work around this issue in the conversion functions, but since the change to nextafter is small enough and the behaviour around zero matches libstdc++, the fix feels at home there.
This commit also converts several variables to unsigned types to avoid undefined behaviour surrounding signed underflow on the subtractions.