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
std.math: Implement more optimal floorImpl for ieeeSingle #5820
Conversation
Thanks for your pull request, @ibuclaw! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + phobos#5820" |
c048355
to
5f81d28
Compare
Ping? |
Pinging people who are qualified to review this from git blame @yebblies @wilzbach @don-clugston-sociomantic @klickverbot |
Or you know, you could just request a review. :) |
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.
I think the code itself is probably OK but it's so cryptic that it's hard to review. I think this low-level stuff really needs copious comments.
|
||
// Other kinds of extractors for real formats. | ||
static if (F.realFormat == RealFormat.ieeeSingle) | ||
int vi; | ||
} | ||
floatBits y = void; | ||
y.rv = x; | ||
|
||
// Find the exponent (power of 2) |
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.
This deserves a much better comment since this code is actually quite tricky.
// Find the exponent (power of 2)
// Do this by shifting the raw value so that the exponent lies in the low bits.
// Then mask out the sign bit, and subtract the bias
std/math.d
Outdated
exp -= 16; | ||
if (exp < (T.mant_dig - 1)) | ||
{ | ||
const uint i = F.MANTISSAMASK_INT >> exp; |
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.
This variable deserves a better name. I think this is actually fraction_mask
, the bits of the mantissa which form the fraction.
std/math.d
Outdated
if ((y.vi & i) != 0) | ||
{ | ||
if (y.vi < 0) | ||
y.vi += 0x00800000 >> exp; |
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.
This REALLY needs a comment. My guess is that this is the float value 0.5, added so that it rounds up towards zero? Or something like that.
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.
This is subtracting one, adding comment.
@redstar - And this you might be interested in also. |
Less branching, and doesn't care about endian-ness. Tested against PPC64/LE.
For comparison between old (source view 1) and new (source view 2) https://explore.dgnu.org/g/fTvttz
Probably better to review with https://github.com/dlang/phobos/pull/5820/files?w=1