APFloat/x87: Fix string conversion for "unnormal" values (pr35860)

[labath]

Summary

Unnormal values are a feature of some very old x87 processors. We handle
them correctly for the most part -- the only exception was an unnormal
value whose significand happened to be zero. In this case the APFloat
was still initialized as normal number (category = fcNormal), but a
subsequent toString operation would assert because the math would
produce nonsensical values for the zero significand.

This commit fixes this by initializing the APFloat to fcZero for these
numbers.

The issue was discovered because LLDB would crash when trying to print
some "long double" values.

[labath]

Ping?

[scanon]

My recollection is that on 386 and later unnormals with a zero significand are treated as an invalid operand; should these print as 0 or as NaN?

[labath]

In D41868#987776, @scanon wrote:

My recollection is that on 386 and later unnormals with a zero significand are treated as an invalid operand; should these print as 0 or as NaN?

I was going off of the comment before initFromF80LongDoubleAPInt, which states that APFloat treats unnormals as regular values (i.e., like a pre-386 processor would). As far as I can tell this statement is true: the non-zero unnormal values will print out correctly (I haven't tried doing arithmetic on them).

So, this only makes sure that we treat "unnormal zero" the same way as other unnormal numbers. However, I can certainly see a case for printing them out differently. This would even be preferable for the debugger use case. OTOH, that will likely be a much more intrusive change than this.

[scanon]

(After checking the architecture manual)

On 387 and later, all unnormals is treated as an "invalid operand", which means they behave the same as a signaling NaN; I think we should probably print them as such. This is a somewhat more invasive change, as you say, but it's more correct behavior.

[labath]

Treat unnormal values as NaN.

It doesn't seem there is anything in the llvm or clang test suite that would
depend on a particular treatment of unnormal values.

[labath]

Ping @scanon. Is this the kind of change you had in mind?

[labath]

@scanon: Could you take another look at this please? This issue is currently causing crashes in LLDB. I could work around this from there, but to me this looks like it should be handled inside APFloat (I haven't checked, but I wouldn't be surprised if it was possible to trigger this from LLVM as well).

If you don't feel comfortable reviewing this, could you suggest another reviewer for this patch?

Thank you.

[scanon]

LGTM

[labath]

Thank you for the review.

llvm/trunk/lib/Support/APFloat.cpp

double IEEEFloat::convertToDouble() const {
   APInt api = bitcastToAPInt();
   return api.bitsToDouble();
 }
 
 /// Integer bit is explicit in this format.  Intel hardware (387 and later)
 /// does not support these bit patterns:
 ///  exponent = all 1's, integer bit 0, significand 0 ("pseudoinfinity")
 ///  exponent = all 1's, integer bit 0, significand nonzero ("pseudoNaN")
-///  exponent = 0, integer bit 1 ("pseudodenormal")
 ///  exponent!=0 nor all 1's, integer bit 0 ("unnormal")
-/// At the moment, the first two are treated as NaNs, the second two as Normal.
+///  exponent = 0, integer bit 1 ("pseudodenormal")
+/// At the moment, the first three are treated as NaNs, the last one as Normal.
 void IEEEFloat::initFromF80LongDoubleAPInt(const APInt &api) {
   assert(api.getBitWidth()==80);
   uint64_t i1 = api.getRawData()[0];
   uint64_t i2 = api.getRawData()[1];
   uint64_t myexponent = (i2 & 0x7fff);
   uint64_t mysignificand = i1;
+  uint8_t myintegerbit = mysignificand >> 63;
 
   initialize(&semX87DoubleExtended);
   assert(partCount()==2);
 
   sign = static_cast<unsigned int>(i2>>15);
-  if (myexponent==0 && mysignificand==0) {
+  if (myexponent == 0 && mysignificand == 0) {
     // exponent, significand meaningless
     category = fcZero;
   } else if (myexponent==0x7fff && mysignificand==0x8000000000000000ULL) {
     // exponent, significand meaningless
     category = fcInfinity;
-  } else if (myexponent==0x7fff && mysignificand!=0x8000000000000000ULL) {
+  } else if ((myexponent == 0x7fff && mysignificand != 0x8000000000000000ULL) ||
+             (myexponent != 0x7fff && myexponent != 0 && myintegerbit == 0)) {
     // exponent meaningless
     category = fcNaN;
     significandParts()[0] = mysignificand;
     significandParts()[1] = 0;
   } else {
     category = fcNormal;
     exponent = myexponent - 16383;
     significandParts()[0] = mysignificand;

llvm/trunk/unittests/ADT/APFloatTest.cpp

TEST(APFloatTest, toString) {
   ASSERT_EQ("0.78539816339744828",
             convertToString(0.78539816339744830961, 0, 3, false));
   ASSERT_EQ("4.94065645841246540e-324",
             convertToString(4.9406564584124654e-324, 0, 3, false));
   ASSERT_EQ("873.18340000000001", convertToString(873.1834, 0, 1, false));
   ASSERT_EQ("8.73183400000000010e+02", convertToString(873.1834, 0, 0, false));
   ASSERT_EQ("1.79769313486231570e+308",
             convertToString(1.7976931348623157E+308, 0, 0, false));
+
+  {
+    SmallString<64> Str;
+    APFloat UnnormalZero(APFloat::x87DoubleExtended(), APInt(80, {0, 1}));
+    UnnormalZero.toString(Str);
+    ASSERT_EQ("NaN", Str);
+  }
 }
 
 TEST(APFloatTest, toInteger) {
   bool isExact = false;
   APSInt result(5, /*isUnsigned=*/true);
 
   EXPECT_EQ(APFloat::opOK,
             APFloat(APFloat::IEEEdouble(), "10")