[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

2706

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

2706

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

2687

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

2704

demorgan this

2711

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) {
-  if (str.equals("inf") || str.equals("INFINITY") || str.equals("+Inf")) {
-    makeInf(false);
+  if (str.size() < 3)
+    return false;
+
+  bool SNaN = false;
+  char NDiff = 'N' - 'Q';
+  char C = str[0];
+  str = str.drop_front();
+  switch (C) {
+  case 'I':
+    // Check for one of: INF, INFINITY.
+    if (str.startswith("NF")) {
+      str = str.drop_front(2);
+      if (!str.empty() && !str.equals("INITY"))
+        return false;
+
+      makeInf(isNegative());
       return true;
     }
+    LLVM_FALLTHROUGH;
+  case 'i':
+    // Check for one of: inf, Inf, infinity, Infinity.
+    if (!str.startswith("nf"))
+      return false;
+
+    str = str.drop_front(2);
+    if (!str.empty() && !str.equals("inity"))
+      return false;
 
-  if (str.equals("-inf") || str.equals("-INFINITY") || str.equals("-Inf")) {
-    makeInf(true);
+    makeInf(isNegative());
     return true;
-  }
 
-  if (str.equals("nan") || str.equals("NaN")) {
-    makeNaN(false, false);
+  case 'S':
+  case 's':
+    SNaN = true;
+    NDiff = 'N' - 'S';
+    LLVM_FALLTHROUGH;
+  case 'Q':
+  case 'q':
+    if (str.size() < 3)
+      return false;
+
+    // If the next character is not 'n' or 'N' (if the last character was 'S' or
+    // 'Q').
+    if (str[0] != 'n' && str[0] != (C + NDiff))
+      return false;
+
+    C = str[0];
+    str = str.drop_front();
+    LLVM_FALLTHROUGH;
+  case 'N':
+  case 'n':
+    // Check for one of: nan, NaN, NAN.
+    if ((str[0] == 'a' || str[0] == (C - ('N' - 'A'))) && str[1] == C) {
+      str = str.drop_front(2);
+      if (str.empty()) {
+        makeNaN(SNaN, isNegative());
         return true;
       }
 
-  if (str.equals("-nan") || str.equals("-NaN")) {
-    makeNaN(false, true);
+      // Allow the payload to be inside parentheses.
+      if (str[0] == '(') {
+        if (!(str.size() > 2 && str.back() == ')'))
+          return false;
+
+        str = str.substr(1, str.size() - 2);
+      }
+
+      // Determine the payload number's radix.
+      unsigned Radix;
+      if (str[0] == '0') {
+        if (str.size() > 2 && (str[1] == 'x' || str[1] == 'X')) {
+          str = str.drop_front(2);
+          Radix = 16;
+        } else
+          Radix = 8;
+      } else
+        Radix = 10;
+
+      // Parse the payload and make the NaN..
+      APInt Payload;
+      if (!str.getAsInteger(Radix, Payload)) {
+        makeNaN(SNaN, isNegative(), &Payload);
         return true;
       }
+    }
+    LLVM_FALLTHROUGH;
 
+  default:
     return false;
   }
+}
 
 Expected<IEEEFloat::opStatus>
 IEEEFloat::convertFromString(StringRef str, roundingMode rounding_mode) {
   if (str.empty())
     return createError("Invalid string length");
 
+  // Handle a leading plus/minus sign.
+  bool Negative = str[0] == '-';
+  if (Negative || str[0] == '+') {
+    str = str.drop_front();
+    if (str.empty())
+      return createError("String has no digits");
+  }
+
+  sign = Negative;
+
   // 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");
-  }
-
-  if (slen >= 2 && p[0] == '0' && (p[1] == 'x' || p[1] == 'X')) {
-    if (slen == 2)
+  if (str.size() >= 2 && str[0] == '0' && (str[1] == 'x' || str[1] == 'X')) {
+    str = str.drop_front(2);
+    if (str.empty())
       return createError("Invalid string");
-    return convertFromHexadecimalString(StringRef(p + 2, slen - 2),
-                                        rounding_mode);
+    return convertFromHexadecimalString(str, rounding_mode);
   }
 
-  return convertFromDecimalString(StringRef(p, slen), rounding_mode);
+  return convertFromDecimalString(str, rounding_mode);
 }
 
 /* Write out a hexadecimal representation of the floating point value
    to DST, which must be of sufficient size, in the C99 form
    [-]0xh.hhhhp[+-]d.  Return the number of characters written,
    excluding the terminating NUL.
 
    If UPPERCASE, the output is in upper case, otherwise in lower case.