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

[spatel]

Summary

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 (AFAIK), a way to specify a 32-bit SNaN value in LLVM IR, so those are always encoded/decoded by calling convert from/to 64-bit hex.

[spatel]

The clang test diffs got dropped from my diff...will update.

[spatel]

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

[spatel]

Patch updated:
Here's an update to the better SNAN fix on top of the previous APFloat patch (we have confirmation that we are no longer miscompiling to Inf in PR43907).
I'm not sure how we would emulate the payload processing in APFloat::convert(), so I just fixed up the quiet bit after we call that.
I have not looked at the background on the MIPS tests, so I don't know if that's ok. The other test diffs appear as expected now.

[efriedma]

Ooh, the MIPS case is weird...

Apparently someone, a while back, made a halfhearted effort at supporting the old MIPS NaN encoding, which reverses the distinction between SNaNs and QNaNs. See http://reviews.llvm.org/D7882 . Of course, it doesn't really work because constant folding doesn't understand the reversal.

Probably you should change clang/test/CodeGen/mips-unsupported-nan.c so it doesn't truncate the result of __builtin_nan(). Otherwise, you're not really making any more broken than it is already.

[spatel]

In D88238#2295903, @efriedma wrote:

Ooh, the MIPS case is weird...

Apparently someone, a while back, made a halfhearted effort at supporting the old MIPS NaN encoding, which reverses the distinction between SNaNs and QNaNs. See http://reviews.llvm.org/D7882 . Of course, it doesn't really work because constant folding doesn't understand the reversal.

Probably you should change clang/test/CodeGen/mips-unsupported-nan.c so it doesn't truncate the result of __builtin_nan(). Otherwise, you're not really making any more broken than it is already.

Thanks for digging that up. I'll add code/comments to explain.

[spatel]

Added code comments to explain the MIPS tests.

[efriedma]

LGTM

[spatel]

D87822 landed recently ( cc @sepavloff ), so this patch is now failing on the clang tests with:

/Users/spatel/GitHub/llvm-project/clang/test/CodeGen/builtin-nan-legacy.c:6:3: error: initializer element is not a compile-time constant
  __builtin_nan(""),
  ^~~~~~~~~~~~~~~~~

[spatel]

In D88238#2300668, @spatel wrote:

D87822 landed recently ( cc @sepavloff ), so this patch is now failing on the clang tests with:

/Users/spatel/GitHub/llvm-project/clang/test/CodeGen/builtin-nan-legacy.c:6:3: error: initializer element is not a compile-time constant
  __builtin_nan(""),
  ^~~~~~~~~~~~~~~~~

I'm not sure what the correct fix will be. The cast from double to float in those tests is raising the exception (opInvalidOp) as expected. That then causes the new APFloat::opStatus check in checkFloatingPointResult() to trigger and cause the error/stop compiling.

[sepavloff]

In D88238#2300685, @spatel wrote:

In D88238#2300668, @spatel wrote:

D87822 landed recently ( cc @sepavloff ), so this patch is now failing on the clang tests with:

/Users/spatel/GitHub/llvm-project/clang/test/CodeGen/builtin-nan-legacy.c:6:3: error: initializer element is not a compile-time constant
  __builtin_nan(""),
  ^~~~~~~~~~~~~~~~~

I'm not sure what the correct fix will be. The cast from double to float in those tests is raising the exception (opInvalidOp) as expected. That then causes the new APFloat::opStatus check in checkFloatingPointResult() to trigger and cause the error/stop compiling.

I would propose to put a check into checkFloatingPointResult like:

if (St & APFloat::opStatus::opInvalidOp) {
  if (E is __builtin_nan() and probably other similar builtins)
    return true;
...

It would allow initialization with NaN but catches other operations with it.

[spatel]

In D88238#2300802, @sepavloff wrote:

I would propose to put a check into checkFloatingPointResult like:

if (St & APFloat::opStatus::opInvalidOp) {
  if (E is __builtin_nan() and probably other similar builtins)
    return true;
...

It would allow initialization with NaN but catches other operations with it.

That seems like a fragile fix - future changes to APFloat behavior might trigger the same error. Is there some reason we are not predicating the error based on the FP exception mode?

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 3bc649b9699..323de80a17d 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -2439,7 +2439,8 @@ static bool checkFloatingPointResult(EvalInfo &Info, const Expr *E,
     return false;
   }
 
-  if (St & APFloat::opStatus::opInvalidOp) {
+  if (St & APFloat::opStatus::opInvalidOp &&
+      FPO.getFPExceptionMode() != LangOptions::FPE_Ignore) {
     // There is no usefully definable result.
     Info.FFDiag(E);
     return false;

[sepavloff]

In D88238#2300890, @spatel wrote:

In D88238#2300802, @sepavloff wrote:

I would propose to put a check into checkFloatingPointResult like:

if (St & APFloat::opStatus::opInvalidOp) {
  if (E is __builtin_nan() and probably other similar builtins)
    return true;
...

It would allow initialization with NaN but catches other operations with it.

That seems like a fragile fix - future changes to APFloat behavior might trigger the same error. Is there some reason we are not predicating the error based on the FP exception mode?

diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 3bc649b9699..323de80a17d 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -2439,7 +2439,8 @@ static bool checkFloatingPointResult(EvalInfo &Info, const Expr *E,
     return false;
   }
 
-  if (St & APFloat::opStatus::opInvalidOp) {
+  if (St & APFloat::opStatus::opInvalidOp &&
+      FPO.getFPExceptionMode() != LangOptions::FPE_Ignore) {
     // There is no usefully definable result.
     Info.FFDiag(E);
     return false;

I have no objection to such solution, if it enables this patch.

Catching Invalid exception in all situations but initialization looks more natural, but this is a topic for other patches.

[spatel]

Patch updated:
Added a check for the FP exception mode before erroring out on APFloat::opStatus::opInvalidOp.

[rjmccall]

The change to ExprConstant should be a different patch, with a test that we can still constant-evaluate something else that results in opInvalid (I assume there must be something?).

[spatel]

In D88238#2301206, @rjmccall wrote:

The change to ExprConstant should be a different patch, with a test that we can still constant-evaluate something else that results in opInvalid (I assume there must be something?).

I can't find an existing path to expose that difference in a test. We use checkFloatingPointResult() in 2 places: FP binops and FP cast. Without this patch, casting never sets opInvalidOp. Binops have a variety of means to create a NaN + opInvalidOp, but that path is gated by:

// [expr.pre]p4:
//   If during the evaluation of an expression, the result is not
//   mathematically defined [...], the behavior is undefined.
// FIXME: C++ rules require us to not conform to IEEE 754 here.
if (LHS.isNaN()) {
  Info.CCEDiag(E, diag::note_constexpr_float_arithmetic) << LHS.isNaN();
  return Info.noteUndefinedBehavior();
}
return checkFloatingPointResult(Info, E, St);

So unless there's a way to raise opInvalidOp without also creating a NaN, we're stuck?

[efriedma]

So unless there's a way to raise opInvalidOp without also creating a NaN, we're stuck?

Maybe you can divide by zero? Otherwise, I agree, we can't split this into a separate patch.

But in any case, we should have some test coverage outside the MIPS-specific testcases.

[spatel]

In D88238#2301878, @efriedma wrote:

So unless there's a way to raise opInvalidOp without also creating a NaN, we're stuck?

Maybe you can divide by zero? Otherwise, I agree, we can't split this into a separate patch.

div-by-zero has its own status bit set in IEEEFloat::divideSpecials():

case PackCategoriesIntoKey(fcNormal, fcZero):
  category = fcInfinity;
  return opDivByZero;

But in any case, we should have some test coverage outside the MIPS-specific testcases.

Ok - I think the tests will necessarily be similar (FP cast) to what we have already, but we can remove the added complexity of the MIPS option.

[rjmccall]

I think we can split it into a separate patch even if we have no way to test it yet. It's a significantly broader change, at least in theory.

[spatel]

Patch updated:
I added the baseline tests with the NAN convert on non-MIPS targets here:
rG187686bea387
And I added a test comment to describe the difference from this patch along with the updated CHECK line.
The exception mode change is still here, so we can see the complete picture, but I will pre-commit that (still no test diff that I can find) if that's preferred.

[rjmccall]

Hmm. Is there a way to actually get an sNaN in floating-point type? I'm concerned that we might be pushing a formal argument in a way that undermines usability.

[rjmccall]

In D88238#2304308, @rjmccall wrote:

Hmm. Is there a way to actually get an sNaN in floating-point type? I'm concerned that we might be pushing a formal argument in a way that undermines usability.

I mean in a non-double floating-point type. I guess there's __builtin_nansf and so on; for some reason we don't seem to test those as much as we test __builtin_nans.

[spatel]

Added tests that use __builtin_nanf and __builtin_nansf (no changes with this patch as a whole or split into 2 pieces). We allow creating 32-bit NAN directly with those builtins.

This may be confusing because LLVM IR doesn't print 32-bit FP as 32-bit hex. That's the FIXME note in AsmWriter.cpp and hopefully the next step...although I suspect it would mean updating several hundred regression tests.

[efriedma]

LGTM, but please wait to see if @rjmccall has any other comments.

Please make sure the commit message has explanation has some explanation of what the clang change is doing.

[spatel]

Patch updated:
Split the clang AST diff off into its own patch and made this patch depend on it.

clang/test/CodeGen/builtin-nan-exception.c

 
 float f[] = {
   __builtin_nanf(""),
   __builtin_nansf(""),
 };
 
 
 // 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(""),
   __builtin_nans(""),
 };
 
 // CHECK: double 0x7FF8000000000000, double 0x7FF4000000000000
 
 double d[] = {
   __builtin_nan(""),
   __builtin_nans(""),
 };

clang/test/CodeGen/builtin-nan-legacy.c

 // 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(""),
 };
 
 double d[] = {
   __builtin_nan(""),
   __builtin_nans(""),
 };

clang/test/CodeGen/mips-unsupported-nan.c

 // CHECK-MIPS2: warning: ignoring '-mnan=2008' option because the 'mips2' architecture does not support it
 // CHECK-MIPS3: warning: ignoring '-mnan=2008' option because the 'mips3' architecture does not support it
 // CHECK-MIPS4: warning: ignoring '-mnan=2008' option because the 'mips4' architecture does not support it
 // CHECK-MIPS32: warning: ignoring '-mnan=2008' option because the 'mips32' architecture does not support it
 // CHECK-MIPS32R6: warning: ignoring '-mnan=legacy' option because the 'mips32r6' architecture does not support it
 // 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

bool LLParser::ConvertValIDToValue(Type *Ty, ValID &ID, Value *&V,
   case ValID::t_APFloat:
     if (!Ty->isFloatingPointTy() ||
         !ConstantFP::isValueValidForType(Ty, ID.APFloatVal))
       return Error(ID.Loc, "floating point constant invalid for type");
 
     // 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,
                               &Ignored);
       else if (Ty->isBFloatTy())
         ID.APFloatVal.convert(APFloat::BFloat(), APFloat::rmNearestTiesToEven,
                               &Ignored);
       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);
 
     if (V->getType() != Ty)
       return Error(ID.Loc, "floating point constant does not have type '" +
                    getTypeString(Ty) + "'");
 
     return false;

llvm/lib/IR/AsmWriter.cpp

if (&APF.getSemantics() == &APFloat::IEEEsingle() ||
       // Otherwise we could not reparse it to exactly the same value, so we must
       // output the string in hexadecimal format!  Note that loading and storing
       // floating point types changes the bits of NaNs on some hosts, notably
       // x86, so we must not use these types.
       static_assert(sizeof(double) == sizeof(uint64_t),
                     "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);
+        if (IsSNAN) {
+          APInt Payload = apf.bitcastToAPInt();
+          apf = APFloat::getSNaN(APFloat::IEEEdouble(), apf.isNegative(),
+                                 &Payload);
+        }
+      }
       Out << format_hex(apf.bitcastToAPInt().getZExtValue(), 0, /*Upper=*/true);
       return;
     }
 
     // Either half, bfloat or some form of long double.
     // These appear as a magic letter identifying the type, then a
     // fixed number of hex digits.
     Out << "0x";

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.
+    // 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;
+    }
   } else {
     *losesInfo = false;
     fs = opOK;
   }
 
   return fs;
 }
 

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

 ; CHECK-LABEL: @overflow_sitofp(
 ; CHECK-NEXT:    ret float 0x7FF0000000000000
 ;
   %i = sitofp i130 400000000000000000000000000000000000000 to float
   ret float %i
 }
 
 ; 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

 @dnan = constant [3 x i64] [ i64 9223235251041752696, i64 9223235251041752697, i64 9223235250773317239 ], align 8
 @fsnan = constant [3 x i32] [ i32 2139637093, i32 2139637092, i32 2139637094 ]
 @dsnan = constant [3 x i64] [ i64 9220983451228067448, i64 9220983451228067449, i64 9220983450959631991 ], align 8
 @.str = internal constant [10 x i8] c"%08x%08x\0A\00"
 @.str1 = internal constant [6 x i8] c"%08x\0A\00"
 
 @var = external global i32
 
+; SNAN becomes QNAN on fptrunc:
+; 2147228864 = 0x7ffc1cc0 : QNAN
+
 define i32 @main() {
 ; CHECK-LABEL: @main(
 ; CHECK-NEXT:  entry:
 ; 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 2147027116, i32* @var, align 4
 ; CHECK-NEXT:    store volatile i32 -2147483648, 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 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:
   %retval = alloca i32, align 4
   %i = alloca i32, align 4
   %uf = alloca %struct..0anon, align 4
   %ud = alloca %struct..1anon, align 8
   %"alloca point" = bitcast i32 0 to i32

llvm/unittests/ADT/APFloatTest.cpp

TEST(APFloatTest, convert) {
   EXPECT_TRUE(losesInfo);
 
   test = APFloat(APFloat::x87DoubleExtended(), "0xf.fffffffp+28");
   test.convert(APFloat::IEEEdouble(), APFloat::rmNearestTiesToEven, &losesInfo);
   EXPECT_EQ(4294967295.0, test.convertToDouble());
   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());
   test.convert(APFloat::x87DoubleExtended(), APFloat::rmNearestTiesToEven,
                &losesInfo);
   EXPECT_TRUE(test.bitwiseIsEqual(X87QNaN));
   EXPECT_FALSE(losesInfo);
 
   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);
 
   test = APFloat::getQNaN(APFloat::x87DoubleExtended());
   test.convert(APFloat::x87DoubleExtended(), APFloat::rmNearestTiesToEven,
                &losesInfo);
   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);
   status = test.convert(APFloat::IEEEsingle(), APFloat::rmNearestTiesToEven, &losesInfo);
   EXPECT_EQ(0x7fc00000, test.bitcastToAPInt());
   EXPECT_TRUE(losesInfo);
   EXPECT_EQ(status, APFloat::opOK);
 }