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

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

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(""),

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(""),

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("");

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);
 

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;
  }

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;

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

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:

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);