Skip to content
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

Backport logGamma dragonfly fix (from ltsmaster impl) #64

Merged

Conversation

dkgroot
Copy link

@dkgroot dkgroot commented May 7, 2018

See ltsmaster-phobos PR:45
(ie: using slightly less accuracy for DragonFlyBSD)

This somewhat ugly patch does work and i think it was ok for ltsmaster, having it in the master branch is not so nice. According to ISO Standard N1570 we should have been using 'lgamma/lgammal' and 'tgamma/tgammal' instead of gamma/gammal.

Question is this the right repo for this 'fix' or should this go upstream (dlang/phobos)?

@dkgroot dkgroot changed the title Backport logGamma fix using slightly less accuracy for DragonFlyBSD Backport logGamma dragonfly fix (from ltsmaster impl) May 8, 2018
@kinke
Copy link
Member

kinke commented May 10, 2018

So the surrounding asserts all use feqrel(); these 2 are the only ones checked via == and in your case differ in the least significant mantissa bit - I'd say this should definitely be upstreamed then (I mean, replacing ==, no DragonFly special case).

@dkgroot
Copy link
Author

dkgroot commented May 11, 2018

@kinke The change is not required when compiled using DMD (Not sure if they would except). Did you read the comment i made last time ? I did create a c++ program to see if it was a platform specific issue, which it does not seem to be. Someone with more knowledge about these math functions might have a better solution.

I do agree that a 'dragonfly' specific case is ugly and should not be required, so this PR was 'my' way of making this issue known.

@kinke
Copy link
Member

kinke commented May 11, 2018

DMD's log(real x) function is defined as LN2 * log2(x) (x86 yl2x instruction), while LDC uses the LLVM intrinsic, which boils down to calling the C runtime's logl(). It's not the first time this leads to a tiny diff (also due to apparently slightly different C runtime implementations across platforms). As the surrounding asserts allow for up to 3 different mantissa bits, allowing 1 for these 2 asserts seems to make perfect sense.

@kinke
Copy link
Member

kinke commented May 11, 2018

In your C++ test, you used double instead of long double. So try again with that (and use %LA for printf), then you should see a tiny diff between DragonFly's logl() and the Linux one.

@dkgroot
Copy link
Author

dkgroot commented May 11, 2018

@kinke Looks like you have dealt with these issues before. Having to go through upstream will mean these changes will not land in 2.080.0 I guess, or is there a way to expedite this roundtrip ?

@kinke
Copy link
Member

kinke commented May 11, 2018

We can cherry-pick this (preferrably without the DragonFly special case) for the pending LDC 1.10 release, no problem. It should land upstream too, but whether it makes it into 2.080.1 by targeting upstream branch stable is questionable.

@kinke kinke merged commit e34ed9a into ldc-developers:ldc May 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants