[APFloat] prevent NaN morphing into Inf on conversion (PR43907)

[spatel]

Summary

We shift the significand right on a truncation, but that needs to be made NaN-safe: always set at least 1 bit in the significand.
https://llvm.org/PR43907

[rjmccall]

Well, I went and did a whole investigation here, only to come around to the idea that what you're doing might be the most reasonable thing.

IEEE 754-2008 section 6.2.3 says:

An operation that propagates a NaN operand to its result and has a single NaN as an input should produce a NaN with the payload of the input NaN if representable in the destination format.
Conversion of a quiet NaN from a narrower format to a wider format in the same radix, and then back to the same narrower format, should not change the quiet NaN payload in any way except to make it canonical.
Conversion of a quiet NaN to a floating-point format of the same or a different radix that does not allow the payload to be preserved, shall return a quiet NaN that should provide some language-defined diagnostic information.

Also, operations on sNaN are permitted to preserve sNaN (if the result can represent it) but are not permitted to substitute a qNaN without signaling.

The standard is not super-clear about how to think about payloads, but probably the most useful idea for language implementors would be that they're an unsigned number right-aligned in the significand, consisting of all the bits following the qNaN indicator bit (and thus cannot be zero in an sNaN).

If I were interpreting all this completely in the abstract, I'd say it collectively implies that we really cannot implement this correctly on top of a right-shift, and the right implementation would have to be:

• Check whether the payload is representable (i.e. whether truncating it would change any bits).
• If so, truncate it and then preserve the original qNaN indicator bit.
• If not, use a zero payload, preserve the original qNaN indicator bit, and then set the second-highest bit if we started with an sNaN (to preserve the sNaN).

But x86 double->float conversion does not keep the payload right-shifted or do overflow checks; it just shifts the payload around, dropping any extra bits. And x86 is probably the most faithful implementation around, and we probably shouldn't have APFloat diverge from its behavior.

So I think the only quibble here is whether it's okay for this to silently produce a qNaN when it might've started with an sNaN. And arguably the current contract of APFloat is to perform operations as if FP exceptions were disabled, and we'll need a slightly different interface if we want to model exceptions properly. So I think this might just be fine as-is.

Steve Canon might have further thoughts.

[scanon]

If we can do it without complication, it would be best to preserve signaling-ness, because that's the more faithful interpretation of IEEE 754 (even though it _doesn't_ match what the HW does, because the HW can signal and APFloat can't). A general principle (imperfectly adhered to) of IEEE 754 is that conversions on signaling NaNs should _either_ signal and produce a quiet NaN (if possible), or should produce a signaling NaN if no signal is possible.

We _certainly_ shouldn't produce infinity, so any movement here is a big improvement.

[efriedma]

even though it _doesn't_ match what the HW does, because the HW can signal and APFloat can't

APFloat can signal exceptions: they're supposed to be returned in opStatus.

[spatel]

In D87835#2279926, @rjmccall wrote:

So I think the only quibble here is whether it's okay for this to silently produce a qNaN when it might've started with an sNaN. And arguably the current contract of APFloat is to perform operations as if FP exceptions were disabled, and we'll need a slightly different interface if we want to model exceptions properly. So I think this might just be fine as-is.

If we started with a qNaN, then we can't hit the bug, right? Ie, if the MSB of the significand is set, there's no way we could shift that out. If that's correct, then we could just always set the bottom bit? That way sNaN remains sNaN even if lose the payload.

Or maybe that's not true in the case of the x87 or other special formats?

[rjmccall]

In D87835#2280070, @spatel wrote:

In D87835#2279926, @rjmccall wrote:

So I think the only quibble here is whether it's okay for this to silently produce a qNaN when it might've started with an sNaN. And arguably the current contract of APFloat is to perform operations as if FP exceptions were disabled, and we'll need a slightly different interface if we want to model exceptions properly. So I think this might just be fine as-is.

If we started with a qNaN, then we can't hit the bug, right? Ie, if the MSB of the significand is set, there's no way we could shift that out. If that's correct, then we could just always set the bottom bit? That way sNaN remains sNaN even if lose the payload.

Or maybe that's not true in the case of the x87 or other special formats?

I think it's generally true, even on x87.

If we're going to try to stick with an sNaN, I think we should set the second bit in the significand, which will then be stable across formats for the same reason that the qNaN bit is.

[spatel]

Patch updated:

1. Set the 2nd bit of the significand to preserve NaN-ness and signalling.
2. Added TODO comments for potentially better/different solutions.
3. Adjusted tests to provide better coverage of current behavior.

[rjmccall]

llvm/lib/Support/APFloat.cpp

2248

Probably best to clarify:

This can only happen with sNaN, so set the 1st bit after the quiet bit so that we still have an sNaN.

2251

That might be more reasonable than manufacturing an artificial sNaN, yeah. But if we're going to do that, it should happen unconditionally, not just when we ended up with a zero significand.

[spatel]

Patch updated:
Updated code comments; no code or test changes.

[rjmccall]

Okay. If we've decided that preserving an sNaN is the best thing to do, as opposed to making it quiet but setting invalid-op, then this looks good to me. But I would be happier if we just made that decision now (what better time?) and then didn't need the TODO.

[spatel]

In D87835#2290688, @rjmccall wrote:

Okay. If we've decided that preserving an sNaN is the best thing to do, as opposed to making it quiet but setting invalid-op, then this looks good to me. But I would be happier if we just made that decision now (what better time?) and then didn't need the TODO.

I don't have an opinion/experience on the sNaN details, so I'm just going on what's been posted in this review.
@scanon / @efriedma - is this good (enough)?

[efriedma]

I think IEEE754's convertFormat operation transforms sNaN->qNaN; I think it makes sense for APFloat to do the same.

[spatel]

In D87835#2291094, @efriedma wrote:

I think IEEE754's convertFormat operation transforms sNaN->qNaN; I think it makes sense for APFloat to do the same.

There's a potential snag:
LLVM IR doesn't allow single-precision (32-bit) FP hex values (not sure why) - we convert those from/to 64-bit as shown in the first test diff in this patch.
That means the following -instsimplify test calls APFloat::convert() with SNaN, so the naive fix would set the quiet bit as shown in the CHECK line before we ever get to -instsimplify in this test:

define float @fsub_snan_op1(float %x) {
; CHECK-LABEL: @fsub_nan_op1(
; CHECK-NEXT:    ret float 0x7FF9000000000000
;
  %r = fsub float %x, 0x7FF1000000000000
  ret float %r
}

We could say that's as intended, but we will then be inconsistent for float vs. every other FP type which can specify some form of SNaN in hex. (That's because -instsimplify does not explicitly set the quiet bit in its normal transforms currently.)
It's possible that we can fall back to the argument that non-constrained LLVM FP ignores FP exceptions, so it doesn't matter.
There are a few other regression tests failing, and I'm not sure what to make of them. I'll post the alternate patch alongside this one.

[rjmccall]

Okay. That honestly seems like something that should just be fixed — it's easy to sniff the literal length to figure out which format we have. But it's probably reasonable to preserve sNaNs in the short term and then come back to this.

[spatel]

Patch updated:

1. Assert that losesInfo was set as expected if we lost the entire NaN payload.
2. Change code comment from 'TODO' to 'FIXME' based on discussion here.
3. Added APFloat unittests for better coverage of losesInfo and status.

[spatel]

Updated diff to include clang tests that fail with the setting of the quiet bit.

[spatel]

Ignore the prior comment/update - I got the review numbers inverted. Sorry about that.

[rjmccall]

LGTM. Although another option would just be to change how we do this "conversion" at parse time.

[efriedma]

LGTM

LLVM IR doesn't allow single-precision (32-bit) FP hex values (not sure why) - we convert those from/to 64-bit as shown in the first test diff in this patch.

Okay. That honestly seems like something that should just be fixed — it's easy to sniff the literal length to figure out which format we have. But it's probably reasonable to preserve sNaNs in the short term and then come back to this.

Currently, we use prefixes: "H", "K", "L", "M", "R" to indicate the floating-point format. We have a prefix for every format except double and single. An unprefixed hex constant is assumed to be in double-precision format; if the constant is in some narrower format, it's converted. (Note this is an issue specifically with the text format; bitcode doesn't do this sort of conversion.)

I think we should do two things here:

1. For reading IR, the conversion shouldn't be using IEEE semantics. We should preserve SNaNs, and forbid any conversion that's not exact.
2. Maybe add a prefix for single-precision constants, so we can write them with 32 bits, and avoid this sort of issue in the future.

But we can deal with that as a followup.

llvm/lib/Support/APFloat.cpp

IEEEFloat::opStatus IEEEFloat::convert(const fltSemantics &toSemantics,
   } else if (category == fcNaN) {
     *losesInfo = lostFraction != lfExactlyZero || X86SpecialNan;
 
     // For x87 extended precision, we want to make a NaN, not a special NaN if
     // the input wasn't special either.
     if (!X86SpecialNan && semantics == &semX87DoubleExtended)
       APInt::tcSetBit(significandParts(), semantics->precision - 1);
 
+    // If we are truncating NaN, it is possible that we shifted out all of the
+    // set bits in a signalling NaN payload. But NaN must remain NaN, so some
+    // bit in the significand must be set (otherwise it is Inf).
+    // This can only happen with sNaN. Set the 1st bit after the quiet bit,
+    // so that we still have an sNaN.
+    // FIXME: Set quiet and return opInvalidOp (on convert of any sNaN).
+    //        But this requires fixing LLVM to parse 32-bit hex FP or ignoring
+    //        conversions while parsing IR.
+    if (APInt::tcIsZero(significandParts(), newPartCount)) {
+      assert(shift < 0 && "Should not lose NaN payload on extend");
+      assert(semantics->precision >= 3 && "Unexpectedly narrow significand");
+      assert(*losesInfo && "Missing payload should have set lost info");
+      APInt::tcSetBit(significandParts(), semantics->precision - 3);
+    }
+
     // gcc forces the Quiet bit on, which means (float)(double)(float_sNan)
     // does not give you back the same bits.  This is dubious, and we
     // don't currently do it.  You're really supposed to get
     // an invalid operation signal at runtime, but nobody does that.
     fs = opOK;
   } else {
     *losesInfo = false;
     fs = opOK;

llvm/test/Transforms/InstSimplify/ConstProp/cast.ll

 define float @overflow_sitofp() {
 ; CHECK-LABEL: @overflow_sitofp(
 ; CHECK-NEXT:    ret float 0x7FF0000000000000
 ;
   %i = sitofp i130 400000000000000000000000000000000000000 to float
   ret float %i
 }
 
-; https://llvm.org/PR43907
+; https://llvm.org/PR43907 - make sure that NaN doesn't morph into Inf.
+; SNaN remains SNaN.
 
 define float @nan_f64_trunc() {
 ; CHECK-LABEL: @nan_f64_trunc(
-; CHECK-NEXT:    ret float 0x7FF0000000000000
+; CHECK-NEXT:    ret float 0x7FF4000000000000
 ;
   %f = fptrunc double 0x7FF0000000000001 to float
   ret float %f
 }
 
+; Verify again with a vector and different destination type.
+; SNaN remains SNaN (first two elements).
+; QNaN remains QNaN (third element).
+; Lower 42 bits of NaN source payload are lost.
+
 define <3 x half> @nan_v3f64_trunc() {
 ; CHECK-LABEL: @nan_v3f64_trunc(
-; CHECK-NEXT:    ret <3 x half> <half 0xH7C00, half 0xH7C00, half 0xH7E00>
+; CHECK-NEXT:    ret <3 x half> <half 0xH7D00, half 0xH7D00, half 0xH7E00>
 ;
   %f = fptrunc <3 x double> <double 0x7FF0020000000000, double 0x7FF003FFFFFFFFFF, double 0x7FF8000000000001> to <3 x half>
   ret <3 x half> %f
 }

llvm/unittests/ADT/APFloatTest.cpp

TEST(APFloatTest, convert) {
   EXPECT_FALSE(losesInfo);
 
   test = APFloat::getQNaN(APFloat::x87DoubleExtended());
   test.convert(APFloat::x87DoubleExtended(), APFloat::rmNearestTiesToEven,
                &losesInfo);
   EXPECT_TRUE(test.bitwiseIsEqual(X87QNaN));
   EXPECT_FALSE(losesInfo);
 
-  // FIXME: This is wrong - NaN becomes Inf.
+  // The payload is lost in truncation, but we must retain NaN, so we set the bit after the quiet bit.
   APInt payload(52, 1);
   test = APFloat::getSNaN(APFloat::IEEEdouble(), false, &payload);
   status = test.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven, &losesInfo);
-  EXPECT_EQ(0x7f800000, test.bitcastToAPInt());
+  EXPECT_EQ(0x7fa00000, test.bitcastToAPInt());
   EXPECT_TRUE(losesInfo);
   EXPECT_EQ(status, APFloat::opOK);
 
+  // The payload is lost in truncation. QNaN remains QNaN.
   test = APFloat::getQNaN(APFloat::IEEEdouble(), false, &payload);
   status = test.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven, &losesInfo);
   EXPECT_EQ(0x7fc00000, test.bitcastToAPInt());
   EXPECT_TRUE(losesInfo);
   EXPECT_EQ(status, APFloat::opOK);
 }
 
 TEST(APFloatTest, PPCDoubleDouble) {