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

[Sanjay Patel via Phabricator via llvm-commits]

spatel updated this revision to Diff 294088.
spatel added a comment.
Herald added a subscriber: dexonsmith.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87835/new/

https://reviews.llvm.org/D87835

Files:
  llvm/lib/Support/APFloat.cpp
  llvm/test/Transforms/InstSimplify/ConstProp/cast.ll
  llvm/unittests/ADT/APFloatTest.cpp


Index: llvm/unittests/ADT/APFloatTest.cpp
===================================================================
--- llvm/unittests/ADT/APFloatTest.cpp
+++ llvm/unittests/ADT/APFloatTest.cpp
@@ -1841,14 +1841,15 @@
   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());
Index: llvm/test/Transforms/InstSimplify/ConstProp/cast.ll
===================================================================
--- llvm/test/Transforms/InstSimplify/ConstProp/cast.ll
+++ llvm/test/Transforms/InstSimplify/ConstProp/cast.ll
@@ -39,19 +39,25 @@
   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
Index: llvm/lib/Support/APFloat.cpp
===================================================================
--- llvm/lib/Support/APFloat.cpp
+++ llvm/lib/Support/APFloat.cpp
@@ -2242,6 +2242,21 @@
     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


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D87835.294088.patch
Type: text/x-patch
Size: 3703 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200924/9c63eb67/attachment.bin>