[APFloat] convert SNaN to QNaN in convert() and raise Invalid signal

rotateright · rotateright · commit 149f5b573c79 · 2020-10-01T14:37:38.000-04:00
This is an alternate fix (see D87835) for a bug where a NaN constant gets wrongly transformed into Infinity via truncation. In this patch, we uniformly convert any SNaN to QNaN while raising 'invalid op'. But we don't have a way to directly specify a 32-bit SNaN value in LLVM IR, so those are always encoded/decoded by calling convert from/to 64-bit hex. See D88664 for a clang fix needed to allow this change. Differential Revision: https://reviews.llvm.org/D88238
diff --git a/clang/test/CodeGen/builtin-nan-exception.c b/clang/test/CodeGen/builtin-nan-exception.c
@@ -17,8 +17,12 @@ float f[] = {
 
 
 // Doubles are created and converted to floats.
+// Converting (truncating) to float quiets the NaN (sets the MSB
+// of the significand) and raises the APFloat invalidOp exception
+// but that should not cause a compilation error in the default
+// (ignore FP exceptions) mode.
 
-// CHECK: float 0x7FF8000000000000, float 0x7FF4000000000000
+// CHECK: float 0x7FF8000000000000, float 0x7FFC000000000000
 
 float converted_to_float[] = {
   __builtin_nan(""),
diff --git a/clang/test/CodeGen/builtin-nan-legacy.c b/clang/test/CodeGen/builtin-nan-legacy.c
@@ -1,7 +1,15 @@
 // RUN: %clang -target mipsel-unknown-linux -mnan=legacy -emit-llvm -S %s -o - | FileCheck %s
-// CHECK: float 0x7FF4000000000000, float 0x7FF8000000000000
+// CHECK: float 0x7FFC000000000000, float 0x7FF8000000000000
 // CHECK: double 0x7FF4000000000000, double 0x7FF8000000000000
 
+// The first line shows an unintended consequence.
+// __builtin_nan() creates a legacy QNAN double with an empty payload
+// (the first bit of the significand is clear to indicate quiet, so
+// the second bit of the payload is set to maintain NAN-ness).
+// The value is then truncated, but llvm::APFloat does not know about
+// the inverted quiet bit, so it sets the first bit on conversion
+// to indicate 'quiet' independently of the setting in clang.
+
 float f[] = {
   __builtin_nan(""),
   __builtin_nans(""),
diff --git a/clang/test/CodeGen/mips-unsupported-nan.c b/clang/test/CodeGen/mips-unsupported-nan.c
@@ -39,7 +39,21 @@
 // CHECK-MIPS64: warning: ignoring '-mnan=2008' option because the 'mips64' architecture does not support it
 // CHECK-MIPS64R6: warning: ignoring '-mnan=legacy' option because the 'mips64r6' architecture does not support it
 
-// CHECK-NANLEGACY: float 0x7FF4000000000000
+// This call creates a QNAN double with an empty payload.
+// The quiet bit is inverted in legacy mode: it is clear to indicate QNAN,
+// so the next highest bit is set to maintain NAN (not infinity).
+// In regular (2008) mode, the quiet bit is set to indicate QNAN.
+
+// CHECK-NANLEGACY: double 0x7FF4000000000000
+// CHECK-NAN2008: double 0x7FF8000000000000
+
+double d =  __builtin_nan("");
+
+// This call creates a QNAN double with an empty payload and then truncates.
+// llvm::APFloat does not know about the inverted quiet bit, so it sets the
+// quiet bit on conversion independently of the setting in clang.
+
+// CHECK-NANLEGACY: float 0x7FFC000000000000
 // CHECK-NAN2008: float 0x7FF8000000000000
 
 float f =  __builtin_nan("");
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
@@ -5345,6 +5345,8 @@ bool LLParser::ConvertValIDToValue(Type *Ty, ValID &ID, Value *&V,
     // The lexer has no type info, so builds all half, bfloat, float, and double
     // FP constants as double.  Fix this here.  Long double does not need this.
     if (&ID.APFloatVal.getSemantics() == &APFloat::IEEEdouble()) {
+      // Check for signaling before potentially converting and losing that info.
+      bool IsSNAN = ID.APFloatVal.isSignaling();
       bool Ignored;
       if (Ty->isHalfTy())
         ID.APFloatVal.convert(APFloat::IEEEhalf(), APFloat::rmNearestTiesToEven,
@@ -5355,6 +5357,14 @@ bool LLParser::ConvertValIDToValue(Type *Ty, ValID &ID, Value *&V,
       else if (Ty->isFloatTy())
         ID.APFloatVal.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven,
                               &Ignored);
+      if (IsSNAN) {
+        // The convert call above may quiet an SNaN, so manufacture another
+        // SNaN. The bitcast works because the payload (significand) parameter
+        // is truncated to fit.
+        APInt Payload = ID.APFloatVal.bitcastToAPInt();
+        ID.APFloatVal = APFloat::getSNaN(ID.APFloatVal.getSemantics(),
+                                         ID.APFloatVal.isNegative(), &Payload);
+      }
     }
     V = ConstantFP::get(Context, ID.APFloatVal);
 
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
@@ -1373,9 +1373,19 @@ static void WriteConstantInternal(raw_ostream &Out, const Constant *CV,
                     "assuming that double is 64 bits!");
       APFloat apf = APF;
       // Floats are represented in ASCII IR as double, convert.
-      if (!isDouble)
+      // FIXME: We should allow 32-bit hex float and remove this.
+      if (!isDouble) {
+        // A signaling NaN is quieted on conversion, so we need to recreate the
+        // expected value after convert (quiet bit of the payload is clear).
+        bool IsSNAN = apf.isSignaling();
         apf.convert(APFloat::IEEEdouble(), APFloat::rmNearestTiesToEven,
-                          &ignored);
+                    &ignored);
+        if (IsSNAN) {
+          APInt Payload = apf.bitcastToAPInt();
+          apf = APFloat::getSNaN(APFloat::IEEEdouble(), apf.isNegative(),
+                                 &Payload);
+        }
+      }
       Out << format_hex(apf.bitcastToAPInt().getZExtValue(), 0, /*Upper=*/true);
       return;
     }
diff --git a/llvm/lib/Support/APFloat.cpp b/llvm/lib/Support/APFloat.cpp
@@ -2243,26 +2243,15 @@ IEEEFloat::opStatus IEEEFloat::convert(const fltSemantics &toSemantics,
     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);
+    // Convert of sNaN creates qNaN and raises an exception (invalid op).
+    // This also guarantees that a sNaN does not become Inf on a truncation
+    // that loses all payload bits.
+    if (isSignaling()) {
+      makeQuiet();
+      fs = opInvalidOp;
+    } else {
+      fs = opOK;
     }
-
-    // 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;
diff --git a/llvm/test/Transforms/InstSimplify/ConstProp/cast.ll b/llvm/test/Transforms/InstSimplify/ConstProp/cast.ll
@@ -40,24 +40,24 @@ define float @overflow_sitofp() {
 }
 
 ; https://llvm.org/PR43907 - make sure that NaN doesn't morph into Inf.
-; SNaN remains SNaN.
+; SNaN becomes QNaN.
 
 define float @nan_f64_trunc() {
 ; CHECK-LABEL: @nan_f64_trunc(
-; CHECK-NEXT:    ret float 0x7FF4000000000000
+; CHECK-NEXT:    ret float 0x7FF8000000000000
 ;
   %f = fptrunc double 0x7FF0000000000001 to float
   ret float %f
 }
 
 ; Verify again with a vector and different destination type.
-; SNaN remains SNaN (first two elements).
+; SNaN becomes 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 0xH7D00, half 0xH7D00, half 0xH7E00>
+; CHECK-NEXT:    ret <3 x half> <half 0xH7E00, half 0xH7E00, half 0xH7E00>
 ;
   %f = fptrunc <3 x double> <double 0x7FF0020000000000, double 0x7FF003FFFFFFFFFF, double 0x7FF8000000000001> to <3 x half>
   ret <3 x half> %f
diff --git a/llvm/test/Transforms/PhaseOrdering/X86/nancvt.ll b/llvm/test/Transforms/PhaseOrdering/X86/nancvt.ll
@@ -18,6 +18,9 @@ target triple = "i686-apple-darwin8"
 
 @var = external global i32
 
+; SNAN becomes QNAN on fptrunc:
+; 2147228864 = 0x7ffc1cc0 : QNAN
+
 define i32 @main() {
 ; CHECK-LABEL: @main(
 ; CHECK-NEXT:  entry:
@@ -30,15 +33,15 @@ define i32 @main() {
 ; CHECK-NEXT:    store volatile i32 2147228864, i32* @var, align 4
 ; CHECK-NEXT:    store volatile i32 2147228864, i32* @var, align 4
 ; CHECK-NEXT:    store volatile i32 2147228864, i32* @var, align 4
-; CHECK-NEXT:    store volatile i32 2146502828, i32* @var, align 4
+; CHECK-NEXT:    store volatile i32 2147027116, i32* @var, align 4
 ; CHECK-NEXT:    store volatile i32 -1610612736, i32* @var, align 4
-; CHECK-NEXT:    store volatile i32 2146502828, i32* @var, align 4
+; CHECK-NEXT:    store volatile i32 2147027116, i32* @var, align 4
 ; CHECK-NEXT:    store volatile i32 -2147483648, i32* @var, align 4
-; CHECK-NEXT:    store volatile i32 2146502828, i32* @var, align 4
+; CHECK-NEXT:    store volatile i32 2147027116, i32* @var, align 4
 ; CHECK-NEXT:    store volatile i32 -1073741824, i32* @var, align 4
-; CHECK-NEXT:    store volatile i32 2143034560, i32* @var, align 4
-; CHECK-NEXT:    store volatile i32 2143034560, i32* @var, align 4
-; CHECK-NEXT:    store volatile i32 2143034560, i32* @var, align 4
+; CHECK-NEXT:    store volatile i32 2147228864, i32* @var, align 4
+; CHECK-NEXT:    store volatile i32 2147228864, i32* @var, align 4
+; CHECK-NEXT:    store volatile i32 2147228864, i32* @var, align 4
 ; CHECK-NEXT:    ret i32 undef
 ;
 entry:
diff --git a/llvm/unittests/ADT/APFloatTest.cpp b/llvm/unittests/ADT/APFloatTest.cpp
@@ -1816,11 +1816,12 @@ TEST(APFloatTest, convert) {
   EXPECT_FALSE(losesInfo);
 
   test = APFloat::getSNaN(APFloat::IEEEsingle());
-  APFloat X87SNaN = APFloat::getSNaN(APFloat::x87DoubleExtended());
   APFloat::opStatus status = test.convert(APFloat::x87DoubleExtended(), APFloat::rmNearestTiesToEven, &losesInfo);
-  EXPECT_TRUE(test.bitwiseIsEqual(X87SNaN));
+  // Conversion quiets the SNAN, so now 2 bits of the 64-bit significand should be set.
+  APInt topTwoBits(64, 0x6000000000000000);
+  EXPECT_TRUE(test.bitwiseIsEqual(APFloat::getQNaN(APFloat::x87DoubleExtended(), false, &topTwoBits)));
   EXPECT_FALSE(losesInfo);
-  EXPECT_EQ(status, APFloat::opOK);
+  EXPECT_EQ(status, APFloat::opInvalidOp);
 
   test = APFloat::getQNaN(APFloat::IEEEsingle());
   APFloat X87QNaN = APFloat::getQNaN(APFloat::x87DoubleExtended());
@@ -1832,6 +1833,7 @@ TEST(APFloatTest, convert) {
   test = APFloat::getSNaN(APFloat::x87DoubleExtended());
   test.convert(APFloat::x87DoubleExtended(), APFloat::rmNearestTiesToEven,
                &losesInfo);
+  APFloat X87SNaN = APFloat::getSNaN(APFloat::x87DoubleExtended());
   EXPECT_TRUE(test.bitwiseIsEqual(X87SNaN));
   EXPECT_FALSE(losesInfo);
 
@@ -1841,13 +1843,13 @@ TEST(APFloatTest, convert) {
   EXPECT_TRUE(test.bitwiseIsEqual(X87QNaN));
   EXPECT_FALSE(losesInfo);
 
-  // The payload is lost in truncation, but we must retain NaN, so we set the bit after the quiet bit.
+  // The payload is lost in truncation, but we retain NaN by setting the quiet bit.
   APInt payload(52, 1);
   test = APFloat::getSNaN(APFloat::IEEEdouble(), false, &payload);
   status = test.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven, &losesInfo);
-  EXPECT_EQ(0x7fa00000, test.bitcastToAPInt());
+  EXPECT_EQ(0x7fc00000, test.bitcastToAPInt());
   EXPECT_TRUE(losesInfo);
-  EXPECT_EQ(status, APFloat::opOK);
+  EXPECT_EQ(status, APFloat::opInvalidOp);
 
   // The payload is lost in truncation. QNaN remains QNaN.
   test = APFloat::getQNaN(APFloat::IEEEdouble(), false, &payload);
