Add DragonFly bootstrap support (ltsmaster) #45

dkgroot · 2017-12-21T17:53:32Z

Related PR's:

ldc
- ldc:ltsmaster
- druntime:ltsmaster
- phobos:ltsmaster
- dmd-testsuite:ltsmaster
dlang:
- dlang/dmd:master
- dlang/druntime:master
- dlang/phobos:master (merged already)

joakim-noah

Much of this pull is from upstream, but some stuff seems off.

joakim-noah · 2017-12-22T17:42:27Z

etc/c/zlib/gzguts.h

@@ -25,6 +25,9 @@
 #  include <stdlib.h>
 #  include <limits.h>
 #endif
+#if __DragonFly__
+#  include <unistd.h>
+#endif


This wasn't needed for dmd upstream?

These two zlib implementation are not directly comparable, they differ quite a bit. The upstream does compile without modification, this older version however does not. The function declarations for 'open', 'close', 'read' and 'write' are defined in unistd.h on DragonFly.

@dkgroot This change shouldn't be needed, see madler/zlib#300 and ldc-developers/ldc#2317

@JohanEngelen I will backport revision 2317 to include this fix. Thanks

Don't worry about that, I will cherry pick that commit to this branch. Once that's in, just make sure it's enough and then take this out. Update: Done.

I rebased the dragonfly-ltsmaster branch to get this change in. Rerunning all tests now (should be ok though).
@joakim-noah I can revert the gzguts.h change if that is easier for you.

joakim-noah · 2017-12-22T17:56:08Z

std/datetime.d

-            else version(OSX)     enum utcZone = "UTC";
-            else static assert(0, "The location of the UTC timezone file on this Posix platform must be set.");
+            version(FreeBSD)           enum utcZone = "Etc/UTC";
+            else version(DragonFlyBSD) enum utcZone = "Etc/UTC";


You set this to UTC in the upstream pull.

joakim-noah · 2017-12-22T18:03:13Z

std/process.d

@@ -1452,7 +1452,7 @@ void kill(Pid pid, int codeOrSignal)
    else version (Posix)
    {
        import core.sys.posix.signal;
-        if (kill(pid.osHandle, codeOrSignal) == -1)
+        if (core.sys.posix.signal.kill(pid.osHandle, codeOrSignal) == -1)


Didn't need this for dmd upstream? I find it hard to believe this symbol scoping affects no other platform but DragonFly.

Reverted... It was needed when running the unittest under dmd-v2.068.1, but apparently not with ltsmaster.

joakim-noah · 2017-12-22T18:04:01Z

std/c/dragonflybsd/socket.d

+public import core.sys.posix.sys.socket : AF_APPLETALK, AF_IPX, SOCK_RDM, MSG_NOSIGNAL;
+public import core.sys.posix.netinet.in_ : IPPROTO_IGMP, IPPROTO_GGP,
+                                          IPPROTO_PUP, IPPROTO_IDP, IPPROTO_ND,
+                                          IPPROTO_MAX, INADDR_LOOPBACK, INADDR_NONE;


This file is unused and worthless, throw it out.

joakim-noah · 2017-12-22T18:08:18Z

std/internal/math/gammafunction.d

+    version(DragonFlyBSD) {} else {  // FIXME
+       assert(logGamma(-50.2) == log(fabs(gamma(-50.2))));
+       assert(logGamma(-0.008) == log(fabs(gamma(-0.008))));
+    }


This wasn't need for dmd upstream? If so, it suggests a problem with the ldc or druntime pulls, likely some math function.

This has not been resolved yet. Would be nice to get some help debugging this one (Maybe from code owner).

Should be pretty easy to start debugging by checking the intermediate results, most likely a loss of precision somewhere. Try running this code with dmd and ldc and see what results you get for each:

void main() { dumpNums(-50.2); dumpNums(-0.008); } void dumpNums(real x) { import std.stdio, std.math, std.internal.math.gammafunction; writefln("logGamma: %g - %A", logGamma(x), logGamma(x)); writefln("gamma: %g - %A, log: %g - %A", gamma(x), gamma(x), log(fabs(gamma(x))), log(fabs(gamma(x)))); }

With dmd 2.077.1 and ldc 1.6 run on linux/x64 with 80-bit reals, I get the following output, including hex formatting for the full precision:

logGamma: -147.586 - -0X9.39620BA62168DB6P+4 gamma: -8.01724e-65 - -0X8.717187D9E6EC13EP-216, log: -147.586 - -0X9.39620BA62168DB6P+4 logGamma: 4.83298 - 0X9.AA7CEB47625307BP-1 gamma: -125.585 - -0XF.B2B9D9745DC2BP+3, log: 4.83298 - 0X9.AA7CEB47625307BP-1

@joakim-noah I already started down this road.

DragonFlyBSD DMD LDC G++

logGamma (%g) -147.586 -147.586 -147.586

logGamma (%A) -0X1.272C4174C42D1B6CP+7 -0X1.272C4174C42D1B6CP+7 -0X1.272C4174C42D3P+7

gamma (%g) -8.01724e-65 -8.01724e-65 -8.01724e-65

gamma (%A) -0X1.0E2E30FB3CDD827CP-213 -0X1.0E2E30FB3CDD827CP-213 -0X1.0E2E30FB3CD69P-213

log(fabs(gamma))) %g -147.586 -147.586 -147.586

log(fabs(gamma))) %A -0X1.272C4174C42D1B6CP+7 -0X1.272C4174C42D1B6AP+7 -0X1.272C4174C42D3P+7

Notice the rounding difference between dmd/ldc on the last line. It looks like the results returned from log() differ between ldc and dmd, the druntime mapping to the c-function is exactly the same, but in phobos/std/math, log is implemented using the llvm_log instrinsic.
Notice that the c++ implementation generates slightly different results (%A).
BTW: DragonFly is running in a VM on a Linux machine (results below), so using the same CPU.

Linux (x86_86) DMD LDC G++

logGamma (%g) -147.586 -147.586 -147.586

logGamma (%A) -0X9.39620BA62168DB6P+4 -0X9.39620BA62168DB6P+4 -0X1.272C4174C42D3P+7

gamma (%g) -8.01724e-65 -8.01724e-65 -8.01724e-65

gamma (%A) -0X8.717187D9E6EC13EP-216 -0X8.717187D9E6EC13EP-216 -0X1.0E2E30FB3CD69P-213

log(fabs(gamma))) %g -147.586 -147.586 -147.586

log(fabs(gamma))) %A -0X9.39620BA62168DB6P+4 -0X9.39620BA62168DB6P+4 -0X1.272C4174C42D3P+7

Notice that c++ delivers the exact same results on Linux and DragonFly. On Linux, the DMD and LDC results do match up.

C++ program used:

#include <iostream> #include <sstream> #include <stdio.h> #include <math.h> int main(int argc, char **argv) { std::stringstream conversionStream(argv[1]); double input; conversionStream >> input; printf("gamma(%f) =%g - %A\n", input, gamma(input), gamma(input)); printf("lgamma(%f) =%g - %A\n", input, lgamma(input), lgamma(input)); printf("tgamma(%f) =%g - %A, log: %g - %A\n", input, tgamma(input), tgamma(input), log(fabs(tgamma(input))), log(fabs(tgamma(input)))); return 0; }

We should be using lgamma/lgammal and tgamma/tgammal instead of gamma/gammal, according to ISO Standard N1570. Also check stackoverflow

OK, if the issue is just some flutter in the last bits because of different log implementations, don't worry about it and take this patch out. It really doesn't matter if this test doesn't pass on DragonFly, simply because it expects exact equality.

Can you explain why running the same functions using g++ / clang++ produces identical output on linux and dragonfly ? But results using dmd and/or ldc differ between the two. Is this something that would be worth investigating ?

How come the llvm_instrinsic log output differs from using platform math.h function ?

But that would mean we "always" fail the unittest (when CI for DFly is in place at some point),
I don't think that is ideal. If necessary we can rewrite the unittest to check with less precision (Not Ideal either).

Your second question answers your first: ldc and dmd are likely using different log functions on DragonFly. As for CI, we can worry about that when it happens, various tests have been made to ignore the last couple bits in std.math already.

The difference probably comes from DMD using an approximation for std.math.log() (based on base-2-log via x87 instruction yl2x, independent from OS) and LDC using the LLVM intrinsic mapping to C function logl(), which may diverge across OS's. I'd expect consistent results (and potentially better performance) when linking in LLVM's builtins library from compiler-rt, which is supposed to provide tuned implementations of some LLVM intrinsics.

@joakim-noah Thanks for your input. I opted to implement a specific version that takes care of the rounding difference, this way the reason for the different handling is clear a preserved for future reference.

joakim-noah · 2018-01-02T09:00:04Z

std/conv.d

-    to!(Test.T)(Test.S());
+    // CHECKME: std/conv.d(513): Warning: calling std.conv.to!(T).to!(S).to without side effects discards return value of type T, prepend a cast(void) if intentional
+    // to!(Test.T)(Test.S());
+    cast(void) to!(Test.T)(Test.S());


I realize you're just pulling in an upstream commit, dd82045, but this warning doesn't trigger for me with ldc ltsmaster on Android/ARM or with ldc 1.6 on linux/x64, with this cast removed. Seems like a pointless addition, rather this were out.

I will retest/recheck if this change is still required, when compiling on DragonFly.

Will remove/revert if not. If it is, we might have to figure out why and/or find some other work around.

joakim-noah · 2018-01-04T05:08:26Z

Squash the last commit and I'll merge.

JohanEngelen · 2018-01-04T10:18:23Z

@joakim-noah you can also do a squash+merge here on github. (squashes all commits into one, you can rewrite the commit message when you do that here) We've been using it a lot.

joakim-noah · 2018-01-04T10:33:02Z

Thanks for the tip: saw the option but didn't know if it'd let me edit the commit text, so told him to do it.

dkgroot · 2018-01-04T12:10:36Z

Thanks guys for the reviews and final merge !!!

See [ltsmaster-phobos PR:45](ldc-developers#45 (comment))

Backport logGamma fix using slightly less accuracy for DragonFlyBSD See [ltsmaster-phobos PR:45](#45 (comment))

joakim-noah suggested changes Dec 22, 2017

View reviewed changes

dkgroot force-pushed the dragonfly-ltsmaster branch from b682518 to 971e30c Compare January 1, 2018 00:41

Add DragonFly support to ldc/phobos

63e493d

dkgroot force-pushed the dragonfly-ltsmaster branch from 971e30c to 63e493d Compare January 1, 2018 02:21

Remove extraneous cast

717cd79

joakim-noah reviewed Jan 2, 2018

View reviewed changes

joakim-noah approved these changes Jan 3, 2018

View reviewed changes

joakim-noah merged commit e60a1f9 into ldc-developers:ldc-ltsmaster Jan 4, 2018

dkgroot added a commit to dkgroot-ldc/phobos that referenced this pull request May 7, 2018

Backport logGamma fix using slightly less accuracy for DragonFlyBSD

14ce671

See [ltsmaster-phobos PR:45](ldc-developers#45 (comment))

dkgroot mentioned this pull request May 7, 2018

Backport logGamma dragonfly fix (from ltsmaster impl) #64

Merged

kinke pushed a commit that referenced this pull request May 12, 2018

Backport logGamma dragonfly fix (from ltsmaster impl) (#64)

e34ed9a

Backport logGamma fix using slightly less accuracy for DragonFlyBSD See [ltsmaster-phobos PR:45](#45 (comment))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DragonFly bootstrap support (ltsmaster) #45

Add DragonFly bootstrap support (ltsmaster) #45

dkgroot commented Dec 21, 2017 •

edited

Loading

joakim-noah left a comment

joakim-noah Dec 22, 2017

dkgroot Dec 27, 2017 •

edited

Loading

JohanEngelen Dec 27, 2017

dkgroot Dec 28, 2017

joakim-noah Dec 28, 2017 •

edited

Loading

dkgroot Dec 28, 2017

joakim-noah Dec 22, 2017

joakim-noah Dec 22, 2017 •

edited

Loading

dkgroot Dec 28, 2017

joakim-noah Dec 22, 2017

joakim-noah Dec 22, 2017

dkgroot Dec 28, 2017

joakim-noah Dec 30, 2017

dkgroot Dec 30, 2017 •

edited

Loading

joakim-noah Dec 30, 2017

dkgroot Dec 30, 2017 •

edited

Loading

joakim-noah Dec 31, 2017

kinke Dec 31, 2017

dkgroot Jan 1, 2018

joakim-noah Jan 2, 2018

dkgroot Jan 2, 2018

joakim-noah commented Jan 4, 2018

JohanEngelen commented Jan 4, 2018

joakim-noah commented Jan 4, 2018

dkgroot commented Jan 4, 2018

DragonFlyBSD	DMD	LDC	G++
logGamma (%g)	-147.586	-147.586	-147.586
logGamma (%A)	-0X1.272C4174C42D1B6CP+7	-0X1.272C4174C42D1B6CP+7	-0X1.272C4174C42D3P+7
gamma (%g)	-8.01724e-65	-8.01724e-65	-8.01724e-65
gamma (%A)	-0X1.0E2E30FB3CDD827CP-213	-0X1.0E2E30FB3CDD827CP-213	-0X1.0E2E30FB3CD69P-213
log(fabs(gamma))) %g	-147.586	-147.586	-147.586
log(fabs(gamma))) %A	-0X1.272C4174C42D1B6CP+7	-0X1.272C4174C42D1B6AP+7	-0X1.272C4174C42D3P+7

Add DragonFly bootstrap support (ltsmaster) #45

Add DragonFly bootstrap support (ltsmaster) #45

Conversation

dkgroot commented Dec 21, 2017 • edited Loading

joakim-noah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkgroot Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakim-noah Dec 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakim-noah Dec 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkgroot Dec 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkgroot Dec 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakim-noah commented Jan 4, 2018

JohanEngelen commented Jan 4, 2018

joakim-noah commented Jan 4, 2018

dkgroot commented Jan 4, 2018

dkgroot commented Dec 21, 2017 •

edited

Loading

dkgroot Dec 27, 2017 •

edited

Loading

joakim-noah Dec 28, 2017 •

edited

Loading

joakim-noah Dec 22, 2017 •

edited

Loading

dkgroot Dec 30, 2017 •

edited

Loading

dkgroot Dec 30, 2017 •

edited

Loading