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

Switch to special quiet NaN as init value for all FP types #2207

Merged
merged 3 commits into from
Aug 4, 2017

Conversation

kinke
Copy link
Member

@kinke kinke commented Jul 14, 2017

Update:

By letting LLVM truncate compile-time real_t values to float/double IR constants. We previously relied on the host, which turns signalling real_t NaNs to quiet float/double NaNs.

Signalling NaNs are characterized by their most-significant mantissa bit being 0 (and at least for D the 2nd-most-significant mantissa bit being set to distinguish them from infinity). The host truncation led to the MSB
getting set while keeping the rest of the mantissa (i.e., both MSBs set).

Here's the resulting bitpattern table in hex:

float double x87 real
init old 7fe00000 7ffc000000000000 7fffa000000000000000
init new 7fa00000 7ff4000000000000 7fffa000000000000000
qnan 7fc00000 7ff8000000000000 7fffc000000000000000

DMD 2.074.0 produces wrongly quiet float/double init values. It's known: https://forum.dlang.org/post/nsp1ql$ivu$1@digitalmars.com

@kinke kinke force-pushed the appveyorHostCompilers branch 3 times, most recently from 118b26c to 95dc0b7 Compare July 14, 2017 18:51
@thewilsonator
Copy link
Contributor

In codegen/vector_init.d on x64 the NaNs in the __initZ symbol seem to initialised to

0x7FF4000000000000

instead of

0x7FFC000000000000

Not sure why, considering it passes on x86. It is the only failure though.

Should we be implementing Target::RealProperties::[s]nan() in C++ as well?

@kinke
Copy link
Member Author

kinke commented Jul 15, 2017

The new value 0x7FF4000000000000 is a signalling NaN, whereas the tested one is a quiet NaN. The inconsistency between 32 and 64 bits is especially ugly (and must be caused by DMD [2.074.0], as that's the first compiler I used to generate the 1.3 LDC bootstrap compiler which then builds 1.3 final).

@kinke kinke changed the title AppVeyor: Upgrade D host compilers Fix signalling-ness of {float,double}.init Jul 16, 2017
@kinke
Copy link
Member Author

kinke commented Jul 16, 2017

[Changed the scope of the PR after figuring out what the actual issue is.]

@dnadlinger
Copy link
Member

Why did you remove isSNaN? Seems like a gratuitous frontend change at first glance.

@kinke
Copy link
Member Author

kinke commented Jul 16, 2017

To make clear we don't need that buggy version assuming x87 real_t if size != 8.

@dnadlinger
Copy link
Member

Maybe at least add a comment saying // buggy; assumes x87 for real.sizeof != 8 or something like that to the version line?

@kinke kinke force-pushed the appveyorHostCompilers branch 2 times, most recently from fe778df to e767f28 Compare July 16, 2017 23:30
@kinke
Copy link
Member Author

kinke commented Jul 18, 2017

Win32 unexpectedly still fails with quiet double.init.

@kinke
Copy link
Member Author

kinke commented Jul 18, 2017

To be more precise: the Win32 LDC compiler generates quiet ones; the 64-bit build with -m32 signalling ones. Possibly something to do with SSE vs. x87.

@kinke kinke changed the title Fix signalling-ness of {float,double}.init Try to fix signalling-ness of {float,double}.init Jul 18, 2017
@kinke
Copy link
Member Author

kinke commented Jul 18, 2017

In the long term, using quiet FP inits (with the second-most significant mantissa bit set too, to distinguish them from normal quiet NaNs) for all 3 types may make more sense.

@kinke
Copy link
Member Author

kinke commented Jul 20, 2017

Well, why wait => switched to quiet {float,double,real}.init.

@kinke
Copy link
Member Author

kinke commented Jul 24, 2017

Any objections to a consistent special quiet NaN for all FP types?

@kinke kinke changed the title Try to fix signalling-ness of {float,double}.init Switch to special quiet NaN as init value for all FP types Jul 24, 2017
@JohanEngelen
Copy link
Member

can't help judging here, maybe ask on the forums if you're unsure.

By letting LLVM narrow compile-time real_t values to float/double IR
constants. We previously relied on the host, which turns signalling
real_t NaNs to quiet float/double NaNs.

Signalling NaNs are characterized by their most-significant mantissa bit
being 0 (and at least for D the 2nd-most-significant mantissa bit being
set to distinguish them from infinity). The host truncation led to the MSB
getting set while keeping the rest of the mantissa (i.e., both MSBs set).

Here's the resulting bitpattern table in hex:

            float:     double:            x87 real:
init old:   7fe00000   7ffc000000000000   7fffa000000000000000
init new:   7fa00000   7ff4000000000000   7fffa000000000000000
qnan:       7fc00000   7ff8000000000000   7fffc000000000000000

DMD 2.074.0 produces wrongly quiet float/double init values.

A Win32 LDC build (with double-precision real_t) still loses the
signalling-ness for {float,double,64-bit real}.init, probably due to the
FPU being used instead of SSE.
As 'processing' a signalling NaN may convert it to a quiet NaN. Apparently
happening on 32-bit x86 using the x87 FPU, but not on x86_64 with SSE.

This fixes potential issues with different constants being used when
mixing natively and cross-compiled objects/libs and failing bitwise `is`
comparisons etc.

See https://forum.dlang.org/post/nsp1ql$ivu$1@digitalmars.com for Martin
Nowak's summarized findings.
@kinke
Copy link
Member Author

kinke commented Aug 4, 2017

No feedback in the forum - merging.

@kinke kinke merged commit 376ca44 into ldc-developers:master Aug 4, 2017
@kinke kinke deleted the appveyorHostCompilers branch August 4, 2017 15:40
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.

4 participants