reimplement longdouble in D by rainers · Pull Request #8169 · dlang/dmd

rainers · 2018-04-14T08:16:54Z

This gets rid of the split implementation in C and D and asm.

Unfortunately, I had to introduce another name for the software implementation, because we now have to cover the case where D code is compiled with dmd using native reals, but needs to supply symbols for the backend that is compiled with VC.

The respective type name aliases are now:
VisualC backend compiler: targ_ldouble typedefs longdouble typedefs longdouble_soft
other C backend compilers: targ_ldouble typedefs longdouble typedefs long double

dmd frontend: real_t aliases longdouble aliases real
LDC/win frontend: real_t aliases longdouble aliases longdouble_soft

The GDC frontend can replace longdouble_soft with a complete software emulation.

dlang-bot · 2018-04-14T08:16:55Z

Thanks for your pull request, @rainers!

Bugzilla references

Your 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 locally

If 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 + dmd#8169"

WalterBright · 2018-04-14T08:57:48Z

src/dmd/root/longdouble.d

 */

-// 80 bit floating point value implementation for LDC compiler targetting MSVC
+// 80-bit floating point value implementation if the C/D compiler do not support them natively


WalterBright · 2018-04-14T08:59:04Z

src/dmd/root/longdouble.d

 {
-nothrow:
+nothrow @nogc:
    ulong mantissa = 0xC000000000000001UL; // default to snan


I gave up on snan a while ago. Nothing supports it, and it just causes problems when people compare nan bits. Use qnan instead.

Ok, updated.

WalterBright · 2018-04-14T09:00:59Z

src/dmd/root/longdouble.d

-    longdouble opMul(longdouble rhs) const { return this.ld_mul(rhs); }
-    longdouble opDiv(longdouble rhs) const { return this.ld_div(rhs); }
-    longdouble opMod(longdouble rhs) const { return this.ld_mod(rhs); }
+    bool opEquals(const longdouble_soft rhs) const { return this.ld_cmpe(rhs); }


hmm, on this rhs is const, but not so for the other functions.

Removed const for consistency as the argumets are passed by value anyway. I abuse the non-constness in some operations to avoid RVO ignoring the asm modifications.

RVO ignoring the asm modifications.

Please file a bug report about that.

Please file a bug report about that.

https://issues.dlang.org/show_bug.cgi?id=18758

WalterBright · 2018-04-14T09:05:18Z

src/dmd/root/longdouble.d

+version(CRuntime_Microsoft):
 extern(C++):
 nothrow:
+@nogc:


Investigate using pure and @safe as much as practical.

Added pure and @safe, but it has pure @trusted has to be verified manually on all the asm.

ibuclaw · 2018-04-14T09:05:52Z

The GDC frontend can replace longdouble_soft with a complete software emulation.

Or I just ignore it completely as neither ctfloat.d nor longdouble.d is shareable code. :-)

https://github.com/ibuclaw/GDC/blob/633751e8146d4533a0a3c17f2440055094008259/gcc/d/ddmd/root/ctfloat.d

By all means I suppose the implementation could be submitted here.

WalterBright · 2018-04-14T09:20:11Z

I used the "Paranoia" test suite to make the DMC soft implementation of floating point arithmetic correct. It lives up to its name, you have to get it right for Paranoia to pass!

http://www.netlib.org/paranoia/paranoia.c

rainers · 2018-04-14T13:52:25Z

I used the "Paranoia" test suite to make the DMC soft implementation of floating point arithmetic correct. It lives up to its name, you have to get it right for Paranoia to pass!

The Paranoia test only seems to fail for the overflow detection (using SIGFPE signals), but I guess we don't need that.

WalterBright · 2018-04-14T20:02:55Z

The Paranoia test only seems to fail for the overflow detection (using SIGFPE signals), but I guess we don't need that.

It's great news that Paranoia is working! Perhaps add it to the test suite? Maintenance on longdouble may accidentally break it. Did you translate it to D?

I know we don't need the signals, but is there a way to get it to pass anyway?

ibuclaw · 2018-04-15T05:46:06Z

The Paranoia test only seems to fail for the overflow detection (using SIGFPE signals), but I guess we don't need that.

It's great news that Paranoia is working! Perhaps add it to the test suite?

Are you testing compile time or runtime here?

rainers · 2018-04-15T07:23:38Z

It's great news that Paranoia is working! Perhaps add it to the test suite? Maintenance on longdouble may accidentally break it. Did you translate it to D?

No, I just added these definitions instead of the float/double versions:

#include "dmd/src/dmd/root/longdouble.h"
#define FLOAT longdouble
#define FABS(x) fabsl(x)
#define FLOOR(x) floor((double)(x))
#define LOG(x) log((double)(x))
#define POW(x,y) pow((double)(x),(double)(y))
#define SQRT(x) sqrtl(x)

and adjusted a couple of explicit conversions from double constants.

I know we don't need the signals, but is there a way to get it to pass anyway?

Not sure. Maybe it just needs enabling approriate FPU exceptions...

rainers · 2018-04-15T07:28:11Z

Are you testing compile time or runtime here?

paranoia tests the C++ interface linked with the D implementation of longdouble, so I'd consider it a test of the compile time evaluation of dmd, but not some D operator overloads.

rainers · 2018-04-15T07:39:52Z

Unfortunately the tests fail because LDC translates all types of FPU instructions to 64-bit, so

    fld real ptr [EAX];
    fld double ptr [EAX];
    fld extended ptr [EAX];

are all the same.
@kinke I see some unexpected translation for extended to double in asm-x86.h for MSVC. I'd rather expect it to be taken literally, but double to be used for real types. Or is this an LLVM limitation?

ibuclaw · 2018-04-15T07:47:56Z

paranoia tests the C++ interface linked with the D implementation of longdouble, so I'd consider it a test of the compile time evaluation of dmd, but not some D operator overloads.

So it could be part of the c++ test source then. Or it could be converted into a ctfe test for the testsuite.

In any case, @WalterBright no chance of supporting all the paranoia tests because there is no fpu at compile time.

WalterBright · 2018-04-15T08:42:28Z

In any case, @WalterBright no chance of supporting all the paranoia tests because there is no fpu at compile time.

The point of paranoia is to test the "no fpu" soft float implementation. I'm not understanding your comment.

ibuclaw · 2018-04-15T09:51:20Z

Why would it raise a signal if floating point is emulated?

kinke · 2018-04-15T12:25:19Z

I see some unexpected translation for extended to double in asm-x86.h for MSVC. I'd rather expect it to be taken literally, but double to be used for real types. Or is this an LLVM limitation?

That's a bug I introduced a while back, thanks for reporting => ldc-developers/ldc#2653.

rainers · 2018-04-15T12:28:23Z

I know we don't need the signals, but is there a way to get it to pass anyway?

Not sure. Maybe it just needs enabling approriate FPU exceptions...

I was wrong about the actual problem due to the rather confusing output: the actual "DEFECT" was an inaccuracy of the pow function, so pretty expected because we are just using the double version.

I then noticed that the FPU precision hasn't been changed at all because I missed to call initFPU() :-/

With that I get 1 "failure" and 3 "defects". Quite ok when compared to dmc (1 failure, 1 serious defect, 6 defects) and gcc (2 failures, 1 serious defect, 6 defects). The latter is rather old, but version 4.9.1 is the latest I found on my system.

Edit: all issues seem to be related to pow and sqrt.

rainers · 2018-04-15T12:34:52Z

That's a bug I introduced a while back, thanks for reporting => ldc-developers/ldc#2653.

Thanks for the quick action. While trying to workaround I noticed

.bytes or db not available to emit the opcodes myself. Would have been nice...
EIP not available as a register (RIP is ok). Is this deliberate?

kinke · 2018-04-15T13:15:22Z

EIP not available as a register (RIP is ok).

Thx => ldc-developers/ldc#2654

.bytes or db not available to emit the opcodes myself. Would have been nice...

I'm not too familiar with the DMD-style inline asm parser code, so I don't know if that would be possible.

rainers · 2018-04-15T13:22:57Z

I'm not too familiar with the DMD-style inline asm parser code, so I don't know if that would be possible.

It seems it is almost there, but currently commented out: https://github.com/ldc-developers/ldc/blob/master/gen/asm-x86.h#L3888

Thx => ldc-developers/ldc#2654

Thanks.

kinke · 2018-04-15T14:34:05Z

[In LLVM asm, loading an 80-bit value would look something like this: import ldc.llvmasm; __asm("fldt $0", "*m,~{st}", ptr);.]

WalterBright · 2018-04-15T20:12:36Z

Why would it raise a signal if floating point is emulated?

Because an emulator should emulate the real thing.

WalterBright · 2018-04-15T20:18:49Z

all issues seem to be related to pow and sqrt.

This is good news, it means the underlying arithmetic is sound. It would be a good idea to make a bugzilla issue of all the paranoia failures. It'd still be grand to convert paranoia to D!

ld_clearfpu() is not pure

… runtime

JinShil · 2018-05-17T05:49:09Z

What's the status with this? @rainers, are you still working on this, or is it ready to go in your opinion? Are there any outstanding issues? What are they?

rainers · 2018-05-17T07:19:56Z

What's the status with this?

I think it's ready to go. A possible complication is that the backend now depends on D code which might make linking a bit more troublesome on non-Windows-platforms (though not used there for now).

wilzbach · 2018-05-17T07:47:20Z

A possible complication is that the backend now depends on D code which might make linking a bit more troublesome on non-Windows-platforms (though not used there for now).

We are about to start converting the backend to D soon hopefully anyhow, so this shouldn't be a big problem.

JinShil · 2018-05-19T01:48:15Z

src/dmd/root/longdouble.d

+    version(D_InlineAsm_X86_64)
+    {
+        // set precision to 64-bit mantissa and rounding control to nearest
+        asm nothrow @nogc pure


nothrow and @nogc are already declared above. Is this redundant? If so, I suggest choosing a coding style: either add the attributes explicitly at each declaration, or group them under one attribution, but don't mix the two styles.

Not confident enough for @trusted?

nothrow and @nogc are already declared above. Is this redundant?

These attributes have to be repeated for each asm statement. otherwise the compiler errors out.

Seems like a bug.

Before dmd 2.067, you couldn't write asm in pure/safe code: https://dlang.org/changelog/2.067.0.html#asm-attributes

JinShil

It's quite beyond me to verify the semantics of this code, but the craftsmanship and translation looks good. Nice work!

JinShil · 2018-05-19T02:00:25Z

src/dmd/root/longdouble.d

+    else version(D_InlineAsm_X86)
+    {
+        // set precision to 64-bit mantissa and rounding control to nearest
+        asm nothrow @nogc


Not pure like the one above?

pure is wrong above, the FPU control word is changed. I'll remove that.

JinShil · 2018-05-19T02:03:03Z

@adamdruppe I understand you're quite good with Intel ASM. Care to lend your expertise here, perhaps even verifying safety and purity?

adamdruppe · 2018-05-19T15:37:33Z

src/dmd/root/longdouble.d

+{
+    version(AsmX86)
+    {
+        asm nothrow @nogc pure @trusted


I probably wouldn't call this pure since it explicitly is clearing flags; if this function were optimized out it would probably be wrong most times.

I'm way out of my league here but if we're talking about floating point flags the spec says [1]:

As a concession to practicality, a pure function can also:

read and write the floating point exception flags

read and write the floating point mode flags, as long as those flags are restored to their initial state upon function entry

[1] https://dlang.org/spec/function.html#pure-functions

adamdruppe · 2018-05-19T15:47:09Z

So I'm not in love with making mov instruction wrappers pure... in isolation, it seems silly, but in context it seems to work. Maaaybe slap private on it just to make it clear this is a bit of internal cheating/convenience, though tbh I am a bit meh on it and wouldn't hold up over this little feeling.

Resetting flag registers while calling it pure rubs me more the wrong way though. I guess it would pass the test if they are pushed and popped inside a pure function, but I don't think they consistently are.

For the @trusted parts.... so it looks right, but I gotta warn my fpu asm experience is fairly limited so I might be missing something too.... just it looks right eyeballing it.

rainers · 2018-05-19T17:47:36Z

Thanks @adamdruppe for review. Indeed, ld_clearfpu() is not pure, so the asm inside should also not be marked that way. I've also added private to the asm mixins.

BTW: there is little new about the asm, it was already there with ldfpu.asm (for Win64) and longdouble.d (for Win32), but sure, merging them might have introduced regressions. Appveyor tests both versions, though.

RazvanN7 · 2018-05-23T13:09:17Z

@WalterBright I think we are ready to merge to this. Do you have anything to add?

rainers requested review from dnadlinger and ibuclaw as code owners April 14, 2018 08:16

rainers force-pushed the longdouble_in_d branch from e5d598f to f081a26 Compare April 14, 2018 08:20

WalterBright reviewed Apr 14, 2018

View reviewed changes

rainers force-pushed the longdouble_in_d branch from 5b0ce1b to e1f92ec Compare April 14, 2018 13:54

rainers force-pushed the longdouble_in_d branch from 8c04fe6 to a5f4836 Compare April 15, 2018 09:15

rainers force-pushed the longdouble_in_d branch from a5f4836 to 33b14f4 Compare April 15, 2018 10:07

rainers force-pushed the longdouble_in_d branch from 68a4668 to 6bd854e Compare April 20, 2018 08:33

rainers added 9 commits April 22, 2018 08:02

reimplement longdouble in D

364ba4d

add pure

1f95441

fix win64.mak

9fed8b1

added @safe and @trusted

19ff22c

ld_clearfpu() is not pure

workaround LDC not emitting tbyte FPU asm instruction

9714c9a

fix comment

ef8dc2a

longdouble_soft: improve operator overloads

1acad05

enable paranoia test, fix output of extended precision values with VC…

adc2773

… runtime

paranoia: enable ExtendedSoft test

8b35cb4

rainers force-pushed the longdouble_in_d branch from 6bd854e to 8b35cb4 Compare April 22, 2018 06:35

JinShil reviewed May 19, 2018

View reviewed changes

JinShil approved these changes May 19, 2018

View reviewed changes

JinShil reviewed May 19, 2018

View reviewed changes

initFPU: remove pure from asm, add @trusted

9b54297

adamdruppe reviewed May 19, 2018

View reviewed changes

ld_clearfpu is not pure, make inline asm mixins private

206f73f

dnadlinger approved these changes May 19, 2018

View reviewed changes

RazvanN7 approved these changes May 23, 2018

View reviewed changes

RazvanN7 added Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Merge:auto-merge labels May 24, 2018

dlang-bot merged commit c470ea6 into dlang:master May 27, 2018

JinShil added the D Conversion label Jun 4, 2018

Uh oh!

Conversation

rainers commented Apr 14, 2018

Uh oh!

dlang-bot commented Apr 14, 2018

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Apr 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WalterBright commented Apr 14, 2018

Uh oh!

rainers commented Apr 14, 2018

Uh oh!

WalterBright commented Apr 14, 2018

Uh oh!

ibuclaw commented Apr 15, 2018

Uh oh!

rainers commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainers commented Apr 15, 2018

Uh oh!

rainers commented Apr 15, 2018

Uh oh!

ibuclaw commented Apr 15, 2018

Uh oh!

WalterBright commented Apr 15, 2018

Uh oh!

ibuclaw commented Apr 15, 2018

Uh oh!

kinke commented Apr 15, 2018

Uh oh!

rainers commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainers commented Apr 15, 2018

Uh oh!

kinke commented Apr 15, 2018

Uh oh!

rainers commented Apr 15, 2018

Uh oh!

kinke commented Apr 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WalterBright commented Apr 15, 2018

Uh oh!

WalterBright commented Apr 15, 2018

Uh oh!

JinShil commented May 17, 2018

Uh oh!

rainers commented May 17, 2018

Uh oh!

wilzbach commented May 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibuclaw commented Apr 14, 2018 •

edited

Loading

rainers commented Apr 15, 2018 •

edited

Loading

rainers commented Apr 15, 2018 •

edited

Loading

kinke commented Apr 15, 2018 •

edited

Loading