[stdlib] Add default payload to Signaling NaN #2494

rintaro · 2016-05-12T03:38:28Z

What's in this pull request?

Or it will have inifinity bit pattern.

CC: @stephentyrone
What the default payload should be? 1 is reasonable?

Before merging this pull request to apple/swift repository:

rintaro · 2016-05-12T05:46:51Z

Or we could do this?:

  public static var signalingNaN: ${Self} {
    return ${Self}(nan: 0, signaling: true)
  }
  public init(nan payload: RawSignificand, signaling: Bool) {
    // We use significandBitCount - 2 for NaN payload.
    precondition(payload < ${Self}._quietNaNMask >> 1,
                 "NaN payload is not encodable.")
    var significand = payload
    significand |= ${Self}._quietNaNMask >> (signaling ? 1 : 0)
    self.init( ... )
  }

FWIW, C++ std::numeric_limits emits:

std::numeric_limits<float>::quiet_NaN()     : 0b01111111110000000000000000000000
std::numeric_limits<float>::signaling_NaN() : 0b01111111101000000000000000000000

stephentyrone · 2016-05-12T14:39:57Z

@swift-ci Please test and merge.

stephentyrone · 2016-05-12T14:43:26Z

@rintaro 1 is as reasonable as _quiet >> 1. There's no standard for what the default sNaN should be. I'd like to rework this further, but let's take this as is for a quick fix.

gribozavr · 2016-05-12T18:34:19Z

stdlib/public/core/FloatingPointTypes.swift.gyb

@@ -459,7 +459,13 @@ extension ${Self}: BinaryFloatingPoint {
    precondition(payload < ${Self}._quietNaNMask,
                 "NaN payload is not encodable.")
    var significand = payload
-    if !signaling { significand |= ${Self}._quietNaNMask }
+    if signaling {
+      precondition(payload != 0,


We can't use precondition() in the library, it won't work correctly. Please use _precondition().

#2504, thank you for the info :)

gribozavr · 2016-05-12T23:04:47Z

@rintaro Could you take a look at the test failure?

rintaro · 2016-05-13T01:55:25Z

@gribozavr I'm looking into it, and will fix it.

@stephentyrone is also working in this area now?
If you found the problem in i386 arch first,
and need to fix this signalingNaN problem as soon as possible,
feel free to close this PR.

rintaro · 2016-05-13T06:12:22Z

I'm struggling to figure out the problem.
With print debugging:

--- a/stdlib/public/core/FloatingPointTypes.swift.gyb
+++ b/stdlib/public/core/FloatingPointTypes.swift.gyb
@@ -191,7 +191,9 @@ extension ${Self}: BinaryFloatingPoint {
   }

   public init(bitPattern: UInt${bits}) {
+    print("bitPattern1: " + String(bitPattern, radix: 2)) 
     self.init(_bits: Builtin.bitcast_Int${bits}_FPIEEE${bits}(bitPattern._value))
+    print("bitPattern2: " + String(self.bitPattern, radix: 2)) 
   }

   @available(*, deprecated, message: "Use the .bitPattern property instead.")
@@ -230,6 +232,7 @@ extension ${Self}: BinaryFloatingPoint {
     self.init(bitPattern: sign << ${RawSignificand}(signShift) |
       exponent << ${RawSignificand}(${Self}.significandBitCount) |
       significand)
+    print("bitPattern3: " + String(self.bitPattern, radix: 2)) 
   }

   public var isCanonical: Bool {

calling Float.signalingNaN on iphonesimulator-x86_64:

bitPattern1: 1111111100000000000000000000001
bitPattern2: 1111111100000000000000000000001
bitPattern3: 1111111100000000000000000000001

calling Float.signalingNaN on iphonesimulator-i386:

bitPattern1: 1111111100000000000000000000001
bitPattern2: 1111111100000000000000000000001
bitPattern3: 1111111110000000000000000000001
                     ^ WHAT???

It looks like, on iphonesimulator-i386, the value is losing signaling-ness
during returning from init(bitPattern:) initializer.
I don't understand how this could happen.

rintaro · 2016-05-13T10:36:57Z

I confirmed with C++, Xcode7, iPhone 4s simulator:

void printme(void *c, size_t n) {
    // ... print bit pattern ...
}

float gen_snan() {
    float a = std::numeric_limits<float>::signaling_NaN();
    printme(&a, sizeof(a));
    return a;
}

void print_snan() {
    float a = gen_snan();
    printme(&a, sizeof(a));
}

output:

01111111101000000000000000000000
01111111111000000000000000000000

Maybe, we cannot support signalingNaN on this platform?

Found this Stackoverflow QA:
http://stackoverflow.com/questions/22816095/signalling-nan-was-corrupted-when-returning-from-x86-function-flds-fstps-of-x87

Or it will have inifinity bit pattern.

rintaro · 2016-05-13T14:41:26Z

Updated the patch.

As a workaround, skipped isSignaling test on i386 arch.
Took 2nd option as std::numeric_limits, because the code looks simpler.

stephentyrone · 2016-05-13T16:05:47Z

test/1_stdlib/Float.swift

@@ -222,13 +222,24 @@ func checkQNaN(_ qnan: TestFloat) {
  _precondition(qnan.floatingPointClass == .quietNaN)
 }

+func checkSNaN(_ snan: TestFloat) {
+  checkNaN(snan)
+// FIXME: i386 doesn't fully support signaling NaN.


FIXME is a bit misleading here; i386 cannot fully support signaling NaN without a massive change to the calling conventions (because loading a Float or Double sNaN to x87 always signals invalid and quiets the NaN). The comment should read something more like:

// sNaN cannot be fully supported on i386.

Thanks, done.

We do own Swift's calling convention. If it were worth it, we could consider changing the convention, for Float80 on x86_64 at least, to pass x87 floats by memory.

On May 13, 2016, at 12:10 PM, Joe Groff notifications@github.com wrote:

In test/1_stdlib/Float.swift #2494 (comment):

@@ -222,13 +222,24 @@ func checkQNaN(_ qnan: TestFloat) {
_precondition(qnan.floatingPointClass == .quietNaN)
}

+func checkSNaN(_ snan: TestFloat) {

checkNaN(snan)
+// FIXME: i386 doesn't fully support signaling NaN.
We do own Swift's calling convention. If it were worth it, we could consider changing the convention, for Float80 on x86_64 at least, to pass x87 floats by memory.

It doesn’t effect Float80 load/store, only Float and Double[*], so there’s no problem on x86_64.

The problem is only on i386, and only for Float and Double. If we wanted to fix it, we would want to return them in xmm0 instead of fp(0).

[*] The history here is interesting. x87 originally did not have load/store of 80-bit values, only 32b and 64b, which were extended to 80b on load and rounded on store. When 80b load/store were added, they were the widest path to memory, so folks wanted to use it for memcpy, which meant that it could not change the values (and hence could not quiet signaling NaNs).

The fractal complexity of x86 never ceases to amaze. If we're willing to assume SSE2, returning in xmm registers on i386 would be a nice improvement…

Yup, it ends up being a ~30% perf win for smallish math functions. The only question is how much anyone cares about i386.

Apple, maybe not so much, but there are still brand-new 32-bit Windows machines being sold today…

https://bugs.swift.org/browse/SR-1515

see: swiftlang#2494

Instead, use `_quietNaNMask >> 1` as the payload. That is, we use only significandBitCount - 2 bits for NaN payload.

stephentyrone · 2016-05-13T16:10:48Z

@swift-ci Please test

stephentyrone · 2016-05-13T17:27:23Z

@rintaro Thanks for following-up with this.

gribozavr · 2016-05-13T17:30:49Z

LGTM, thanks @rintaro @stephentyrone!

* [stdlib] Add default payload to Signaling NaN Or it will have inifinity bit pattern. * [stdlib] Skip isSignaling test on i386 arch see: #2494 * [stdlib] Allow 0 payload for signaling NaN Instead, use `_quietNaNMask >> 1` as the payload. That is, we use only significandBitCount - 2 bits for NaN payload.

gribozavr reviewed May 12, 2016
View reviewed changes

rintaro force-pushed the float-snan-payload branch from 0334325 to 71b906c Compare May 13, 2016 13:06

rintaro force-pushed the float-snan-payload branch 2 times, most recently from c645426 to aa8bbae Compare May 13, 2016 13:08

[stdlib] Add default payload to Signaling NaN

5e3f533

Or it will have inifinity bit pattern.

rintaro force-pushed the float-snan-payload branch from aa8bbae to 88df846 Compare May 13, 2016 13:12

stephentyrone reviewed May 13, 2016
View reviewed changes

rintaro added 2 commits May 14, 2016 01:08

[stdlib] Skip isSignaling test on i386 arch

bc64b48

see: swiftlang#2494

[stdlib] Allow 0 payload for signaling NaN

58c7f59

Instead, use `_quietNaNMask >> 1` as the payload. That is, we use only significandBitCount - 2 bits for NaN payload.

rintaro force-pushed the float-snan-payload branch from 88df846 to 58c7f59 Compare May 13, 2016 16:08

stephentyrone merged commit 949a6b9 into swiftlang:master May 13, 2016

rintaro deleted the float-snan-payload branch May 14, 2016 09:08

rintaro mentioned this pull request Mar 27, 2018

SR-106: New floating-point description implementation #15474

Merged

jckarter mentioned this pull request May 13, 2016

[SR-1515] Consider returning Float/Double in xmm0 on i386 #44124

Open

fumoboy007 mentioned this pull request Aug 28, 2023

Unpacking float32 does not preserve signaling NaN, unlike float64 msgpack/msgpack-c#1087

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stdlib] Add default payload to Signaling NaN #2494

[stdlib] Add default payload to Signaling NaN #2494

rintaro commented May 12, 2016

rintaro commented May 12, 2016 •

edited

Loading

stephentyrone commented May 12, 2016

stephentyrone commented May 12, 2016

gribozavr May 12, 2016

rintaro May 13, 2016

gribozavr commented May 12, 2016

rintaro commented May 13, 2016

rintaro commented May 13, 2016 •

edited

Loading

rintaro commented May 13, 2016 •

edited

Loading

rintaro commented May 13, 2016

stephentyrone May 13, 2016 •

edited

Loading

rintaro May 13, 2016

jckarter May 13, 2016

stephentyrone May 13, 2016

jckarter May 13, 2016

stephentyrone May 13, 2016

jckarter May 13, 2016

stephentyrone May 13, 2016

stephentyrone commented May 13, 2016

stephentyrone commented May 13, 2016

gribozavr commented May 13, 2016

[stdlib] Add default payload to Signaling NaN #2494

[stdlib] Add default payload to Signaling NaN #2494

Conversation

rintaro commented May 12, 2016

What's in this pull request?

rintaro commented May 12, 2016 • edited Loading

stephentyrone commented May 12, 2016

stephentyrone commented May 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gribozavr commented May 12, 2016

rintaro commented May 13, 2016

rintaro commented May 13, 2016 • edited Loading

rintaro commented May 13, 2016 • edited Loading

rintaro commented May 13, 2016

stephentyrone May 13, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentyrone commented May 13, 2016

stephentyrone commented May 13, 2016

gribozavr commented May 13, 2016

rintaro commented May 12, 2016 •

edited

Loading

rintaro commented May 13, 2016 •

edited

Loading

rintaro commented May 13, 2016 •

edited

Loading

stephentyrone May 13, 2016 •

edited

Loading