-
Notifications
You must be signed in to change notification settings - Fork 269
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
Introduce floatbv_round_to_integral_exprt
#8538
base: develop
Are you sure you want to change the base?
Conversation
84b83ec
to
b52d32e
Compare
round_to_integral_exprt
floatbv_round_to_integral_exprt
9c6589a
to
e189f1c
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8538 +/- ##
===========================================
+ Coverage 78.92% 78.94% +0.01%
===========================================
Files 1732 1732
Lines 198958 199206 +248
Branches 18543 18609 +66
===========================================
+ Hits 157037 157266 +229
- Misses 41921 41940 +19 ☔ View full report in Codecov by Sentry. |
f59ad70
to
151d627
Compare
@@ -136,6 +136,32 @@ bvt float_utilst::to_integer( | |||
return result; | |||
} | |||
|
|||
bvt float_utilst::round_to_integral(const bvt &src) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this actually consider the rounding mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It calls add_sub
.
|
||
auto tmp1 = add_sub(src, magic_number_bv, false); | ||
|
||
auto tmp2 = add_sub(tmp1, magic_number_bv, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If ieee_floatt::round_to_integral
is to be trusted, shouldn't the sign bit be used instead of hard-coding false
and true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note line 153 -- this variant tweaks the sign bit of the magic constant instead.
ec1cfc6
to
643372e
Compare
b6b25aa
to
ddeedbc
Compare
This adds a new expression, floatbv_round_to_integral, which rounds an IEEE 754 floating-point number given as bit-vector to the nearest integer, considering the explicitly given rounding mode.
ddeedbc
to
8b80200
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good but could use some comments to explain how fractional bits get cleared due to the exponent alignment with the magic constant, and tests for halfway values and different tie breaking modes
return v; | ||
}; | ||
|
||
auto round_to_integral = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also test for some small and large values halfway between integers, previous below, next above, x (e.g. 0.5, 0.49999999, 0.500000001) and cross that with the different rounding modes so check that ties are actually resolved as expected?
'
This adds a new expression,
floatbv_round_to_integral
, which rounds an IEEE 754 floating-point number given as bit-vector to the nearest integer, considering the explicitly given rounding mode.