-
-
Notifications
You must be signed in to change notification settings - Fork 706
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
fix fabs() to not use x87 for float/double #7561
Conversation
|
Thanks for your pull request, @WalterBright! 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 run digger -- build "master + phobos#7561" |
| ulong tmp = *cast(ulong*)&d & 0x7FFF_FFFF_FFFF_FFFF; | ||
| return *cast(double*)&tmp; |
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.
is this Endian safe?
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 should be fine. I added unittests to verify.
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.
The CI wouldn't really catch that though, as it's all LittleEndian.
However... FYI @thewilsonator, all targets I'm aware of use same endianness for both float and integer words, regardless whether the it's a BigEndian or LittleEndian architecture. The one exception to the rule (there's always one) is the PDP-11, and I doubt Phobos will ever support that. :-)
| uint tmp = *cast(uint*)&f & 0x7FFF_FFFF; | ||
| return *cast(float*)&tmp; |
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.
ditto
|
Since it's still round tripping through the stack Link to compare functions & compilers: https://d.godbolt.org/z/bvd1Ev |
To be fair, the compiler can recognize the pattern and emit btr all by itself. https://d.godbolt.org/z/r5nWM4 |
This is a partial fix for https://issues.dlang.org/show_bug.cgi?id=19663
A complete fix would be to make it a compiler intrinsic.