[APFloat] Extend converting from special strings

[ekatz]

Summary

Add support for converting Signaling NaN, and a NaN Payload from string.

The NaNs (the string "nan" or "NaN") may be prefixed with 's' or 'S' for defining a Signaling NaN.
A payload for a NaN can be specified as a suffix. It may be a octal/decimal/hexadecimal number in parentheses or without.

[hfinkel]

Please add a test case.

[ekatz]

Added unit test.

[ekatz]

Ping

[efriedma]

Are you trying to match some specific grammar here?

We use string->float conversion in a bunch of places, including places where the grammar is specified by some external standard; I'm a little concerned we could be accidentally extending the user-visible grammar in some context. (Maybe those places account for this issue already? Not sure.)

[ekatz]

In D69773#1752248, @efriedma wrote:

Are you trying to match some specific grammar here?

We use string->float conversion in a bunch of places, including places where the grammar is specified by some external standard; I'm a little concerned we could be accidentally extending the user-visible grammar in some context. (Maybe those places account for this issue already? Not sure.)

There is no unified syntax for those "specials"; but these are pretty acceptable representations that I added.
Some are pretty straight forward, like the case of NaN: we had "nan" and "NaN", in many places (in the web) you may see references to it as "NAN", so why not support it as well?
Others are extensions, so we may express "Signaling NaN" or explicit "Quiet NaN". And of course, there is the payload. Why not support it?

I think it is a welcoming extension to LLVM, while Clang (and other frontends) doesn't (and shouldn't) rely on the formats allowed by APFloat, but by its own restrictions.

• Please, also note that SNaN with PAYLOAD is needed for the tests in D69774.

[ekatz]

Ping

[ekatz]

Ping

[hfinkel]

llvm/lib/Support/APFloat.cpp

2702

My impression is that this logic involving NDiff is here to make sure that the case matches, correct? This makes the code hard to follow. Frankly, if this is just a feature for us (to make it easier to write FP numbers in textual LLVM IR), why bother checking for the case matching at all? Just do a case-insensitive match. Otherwise, you can also match and then just check that: isupper(str[0]) == isupper(str[2]), no?

[ekatz]

llvm/lib/Support/APFloat.cpp

2702

I think that the key of your comment is that:

...the code hard to follow.

I agree, and I decided to implement a simpler version (maybe not as efficient, but it is negligible anyway).

[ekatz]

Reduced the patch to only the extensions of:

• Signaling NaN
• NaN Pyaload

[ekatz]

Ping

[ekatz]

Ping

[arsenm]

LGTM with nits

llvm/lib/Support/APFloat.cpp

2647

For only this, I think == 's' or =='S' would be clearer

2666

demorgan this

2673

Initialize to 10 and remove the else case?

[ekatz]

Done requested nit changes.

llvm/lib/Support/APFloat.cpp

/* We can finally safely perform the max-exponent check. */
 
     delete [] decSignificand;
   }
 
   return fs;
 }
 
 bool IEEEFloat::convertFromStringSpecials(StringRef str) {
+  const size_t MIN_NAME_SIZE = 3;
+
+  if (str.size() < MIN_NAME_SIZE)
+    return false;
+
   if (str.equals("inf") || str.equals("INFINITY") || str.equals("+Inf")) {
     makeInf(false);
     return true;
   }
 
-  if (str.equals("-inf") || str.equals("-INFINITY") || str.equals("-Inf")) {
+  bool IsNegative = str.front() == '-';
+  if (IsNegative) {
+    str = str.drop_front();
+    if (str.size() < MIN_NAME_SIZE)
+      return false;
+
+    if (str.equals("inf") || str.equals("INFINITY") || str.equals("Inf")) {
       makeInf(true);
       return true;
     }
+  }
+
+  // If we have a 's' (or 'S') prefix, then this is a Signaling NaN.
+  bool IsSignaling = str.front() == 's' || str.front() == 'S';
+  if (IsSignaling) {
+    str = str.drop_front();
+    if (str.size() < MIN_NAME_SIZE)
+      return false;
+  }
 
-  if (str.equals("nan") || str.equals("NaN")) {
-    makeNaN(false, false);
+  if (str.startswith("nan") || str.startswith("NaN")) {
+    str = str.drop_front(3);
+
+    // A NaN without payload.
+    if (str.empty()) {
+      makeNaN(IsSignaling, IsNegative);
       return true;
     }
 
-  if (str.equals("-nan") || str.equals("-NaN")) {
-    makeNaN(false, true);
+    // Allow the payload to be inside parentheses.
+    if (str.front() == '(') {
+      // Parentheses should be balanced (and not empty).
+      if (str.size() <= 2 || str.back() != ')')
+        return false;
+
+      str = str.slice(1, str.size() - 1);
+    }
+
+    // Determine the payload number's radix.
+    unsigned Radix = 10;
+    if (str[0] == '0') {
+      if (str.size() > 1 && tolower(str[1]) == 'x') {
+        str = str.drop_front(2);
+        Radix = 16;
+      } else
+        Radix = 8;
+    }
+
+    // Parse the payload and make the NaN.
+    APInt Payload;
+    if (!str.getAsInteger(Radix, Payload)) {
+      makeNaN(IsSignaling, IsNegative, &Payload);
       return true;
     }
+  }
 
   return false;
 }
 
 Expected<IEEEFloat::opStatus>
 IEEEFloat::convertFromString(StringRef str, roundingMode rounding_mode) {
   if (str.empty())
     return createError("Invalid string length");
 
   // Handle special cases.
   if (convertFromStringSpecials(str))
     return opOK;
 
   /* Handle a leading minus sign.  */
   StringRef::iterator p = str.begin();
   size_t slen = str.size();
   sign = *p == '-' ? 1 : 0;
   if (*p == '-' || *p == '+') {
     p++;
     slen--;
     if (!slen)
       return createError("String has no digits");

llvm/unittests/ADT/APFloatTest.cpp

TEST(APFloatTest, fromDecimalString) {
   EXPECT_TRUE(APFloat(APFloat::IEEEdouble(), "99e99999").isInfinity());
   EXPECT_TRUE(APFloat(APFloat::IEEEdouble(), "-99e99999").isInfinity());
   EXPECT_TRUE(APFloat(APFloat::IEEEdouble(), "1e-99999").isPosZero());
   EXPECT_TRUE(APFloat(APFloat::IEEEdouble(), "-1e-99999").isNegZero());
 
   EXPECT_EQ(2.71828, convertToDoubleFromString("2.71828"));
 }
 
+TEST(APFloatTest, fromStringSpecials) {
+  const fltSemantics &Sem = APFloat::IEEEdouble();
+  const unsigned Precision = 53;
+  const unsigned PayloadBits = Precision - 2;
+  uint64_t PayloadMask = (uint64_t(1) << PayloadBits) - uint64_t(1);
+
+  uint64_t NaNPayloads[] = {
+      0,
+      1,
+      123,
+      0xDEADBEEF,
+      uint64_t(-2),
+      uint64_t(1) << PayloadBits,       // overflow bit
+      uint64_t(1) << (PayloadBits - 1), // signaling bit
+      uint64_t(1) << (PayloadBits - 2)  // highest possible bit
+  };
+
+  // Convert payload integer to decimal string representation.
+  std::string NaNPayloadDecStrings[array_lengthof(NaNPayloads)];
+  for (size_t I = 0; I < array_lengthof(NaNPayloads); ++I)
+    NaNPayloadDecStrings[I] = utostr(NaNPayloads[I]);
+
+  // Convert payload integer to hexadecimal string representation.
+  std::string NaNPayloadHexStrings[array_lengthof(NaNPayloads)];
+  for (size_t I = 0; I < array_lengthof(NaNPayloads); ++I)
+    NaNPayloadHexStrings[I] = "0x" + utohexstr(NaNPayloads[I]);
+
+  // Fix payloads to expected result.
+  for (uint64_t &Payload : NaNPayloads)
+    Payload &= PayloadMask;
+
+  // Signaling NaN must have a non-zero payload. In case a zero payload is
+  // requested, a default arbitrary payload is set instead. Save this payload
+  // for testing.
+  const uint64_t SNaNDefaultPayload =
+      APFloat::getSNaN(Sem).bitcastToAPInt().getZExtValue() & PayloadMask;
+
+  // Negative sign prefix (or none - for positive).
+  const char Signs[] = {0, '-'};
+
+  // "Signaling" prefix (or none - for "Quiet").
+  const char NaNTypes[] = {0, 's', 'S'};
+
+  const StringRef NaNStrings[] = {"nan", "NaN"};
+  for (StringRef NaNStr : NaNStrings)
+    for (char TypeChar : NaNTypes) {
+      bool Signaling = (TypeChar == 's' || TypeChar == 'S');
+
+      for (size_t J = 0; J < array_lengthof(NaNPayloads); ++J) {
+        uint64_t Payload = (Signaling && !NaNPayloads[J]) ? SNaNDefaultPayload
+                                                          : NaNPayloads[J];
+        std::string &PayloadDec = NaNPayloadDecStrings[J];
+        std::string &PayloadHex = NaNPayloadHexStrings[J];
+
+        for (char SignChar : Signs) {
+          bool Negative = (SignChar == '-');
+
+          std::string TestStrings[5];
+          size_t NumTestStrings = 0;
+
+          std::string Prefix;
+          if (SignChar)
+            Prefix += SignChar;
+          if (TypeChar)
+            Prefix += TypeChar;
+          Prefix += NaNStr;
+
+          // Test without any paylod.
+          if (!Payload)
+            TestStrings[NumTestStrings++] = Prefix;
+
+          // Test with the payload as a suffix.
+          TestStrings[NumTestStrings++] = Prefix + PayloadDec;
+          TestStrings[NumTestStrings++] = Prefix + PayloadHex;
+
+          // Test with the payload inside parentheses.
+          TestStrings[NumTestStrings++] = Prefix + '(' + PayloadDec + ')';
+          TestStrings[NumTestStrings++] = Prefix + '(' + PayloadHex + ')';
+
+          for (size_t K = 0; K < NumTestStrings; ++K) {
+            StringRef TestStr = TestStrings[K];
+
+            APFloat F(Sem);
+            bool HasError = !F.convertFromString(
+                TestStr, llvm::APFloat::rmNearestTiesToEven);
+            EXPECT_FALSE(HasError);
+            EXPECT_TRUE(F.isNaN());
+            EXPECT_EQ(Signaling, F.isSignaling());
+            EXPECT_EQ(Negative, F.isNegative());
+            uint64_t PayloadResult =
+                F.bitcastToAPInt().getZExtValue() & PayloadMask;
+            EXPECT_EQ(Payload, PayloadResult);
+          }
+        }
+      }
+    }
+
+  const StringRef InfStrings[] = {"inf",  "INFINITY",  "+Inf",
+                                  "-inf", "-INFINITY", "-Inf"};
+  for (StringRef InfStr : InfStrings) {
+    bool Negative = InfStr.front() == '-';
+
+    StringRef TestStr;
+    APFloat F(Sem);
+    bool HasError =
+        !F.convertFromString(InfStr, llvm::APFloat::rmNearestTiesToEven);
+    EXPECT_FALSE(HasError);
+    EXPECT_TRUE(F.isInfinity());
+    EXPECT_EQ(Negative, F.isNegative());
+    uint64_t PayloadResult = F.bitcastToAPInt().getZExtValue() & PayloadMask;
+    EXPECT_EQ(0, PayloadResult);
+  }
+}
+
 TEST(APFloatTest, fromToStringSpecials) {
   auto expects = [] (const char *first, const char *second) {
     std::string roundtrip = convertToString(convertToDoubleFromString(second), 0, 3);
     EXPECT_STREQ(first, roundtrip.c_str());
   };
   expects("+Inf", "+Inf");
   expects("+Inf", "INFINITY");
   expects("+Inf", "inf");