[APFloat] Fix comments. NFC.

[timshen]

Summary

Fix comments in response to jlebar's comments in D27872.

[jlebar]

llvm/include/llvm/ADT/APFloat.h

1054

s/one/a/

1055

Nit, this indentation seems noncanonical.

1064

Perhaps "Add two APFloats, rounding ties to the nearest even.", so we repeat less of what's in the code?

1065

Nit, I'm not sure we need the asterisks around "No".

llvm/lib/Support/APFloat.cpp

69

s/It consists/They consist/ to match "number*s*" earlier.

70

Well, that second criterion is only true for non-denormals, right?

72

adjacent to

82

(Here "it's" is fine, because the antecedent is "semantics", used in a singular sense, rather than "numbers".)

4243

Hm, in order for this to be true for denormals, they must also have the same sign, right?

[iteratee]

llvm/lib/Support/APFloat.cpp

73

You could clarify that there are 2 11-bit exponents.

[timshen]

Tweak wording.

llvm/include/llvm/ADT/APFloat.h

1065

Removed. I don't like it either.

llvm/lib/Support/APFloat.cpp

69

Tweaked a bit.

70

Added ", and if normalized, ..."

[jlebar]

lgtm!

llvm/lib/Support/APFloat.cpp

70

I think "if normal" or "if not denormal" is what you want. "if normalized" implies that you may be able to take *any* Hi/Lo and "normalize" them so that this criterion is true. But that's not true for some values (namely, denormals).

Unless we use "normalized" elsewhere in apfloat, in which case, whatever. :)

4340

Again, perhaps "normal" or "non-denormal".

[timshen]

s/normalized/normal/ in the commit. I committed before uploading it to Phab. :P

llvm/lib/Support/APFloat.cpp

70

Changed to normal as APFloat uses it in other places too.

FWIW, https://en.wikipedia.org/wiki/Denormal_number aliases these two terms.

llvm/include/llvm/ADT/APFloat.h

 /// This essentially combines the roles of guard and sticky bits.
 enum lostFraction { // Example of truncated bits:
   lfExactlyZero,    // 000000
   lfLessThanHalf,   // 0xxxxx  x's not all zero
   lfExactlyHalf,    // 100000
   lfMoreThanHalf    // 1xxxxx  x's not all zero
 };
 
-/// \brief A self-contained host- and target-independent arbitrary-precision
+/// A self-contained host- and target-independent arbitrary-precision
 /// floating-point software implementation.
 ///
 /// APFloat uses bignum integer arithmetic as provided by static functions in
 /// the APInt class.  The library will work with bignum integers whose parts are
 /// any unsigned type at least 16 bits wide, but 64 bits is recommended.
 ///
 /// Written for clarity rather than speed, in particular with a view to use in
 /// the front-end of a cross compiler so that target arithmetic can be correctly
enum fltCategory {
     fcZero
   };
 
   /// Convenience enum used to construct an uninitialized APFloat.
   enum uninitializedTag {
     uninitialized
   };
 
-  /// \brief Enumeration of \c ilogb error results.
+  /// Enumeration of \c ilogb error results.
   enum IlogbErrorKinds {
     IEK_Zero = INT_MIN + 1,
     IEK_NaN = INT_MIN,
     IEK_Inf = INT_MAX
   };
 
   static unsigned int semanticsPrecision(const fltSemantics &);
   static ExponentType semanticsMinExponent(const fltSemantics &);
public:
   explicit IEEEFloat(double d);
   explicit IEEEFloat(float f);
   IEEEFloat(const IEEEFloat &);
   IEEEFloat(IEEEFloat &&);
   ~IEEEFloat();
 
   /// @}
 
-  /// \brief Returns whether this instance allocated memory.
+  /// Returns whether this instance allocated memory.
   bool needsCleanup() const { return partCount() > 1; }
 
   /// \name Convenience "constructors"
   /// @{
 
   /// @}
 
   /// \name Arithmetic
public:
   /// Returns true if and only if the number is an exact integer.
   bool isInteger() const;
 
   /// @}
 
   IEEEFloat &operator=(const IEEEFloat &);
   IEEEFloat &operator=(IEEEFloat &&);
 
-  /// \brief Overload to compute a hash code for an APFloat value.
+  /// Overload to compute a hash code for an APFloat value.
   ///
   /// Note that the use of hash codes for floating point values is in general
   /// frought with peril. Equality is hard to define for these values. For
   /// example, should negative and positive zero hash to different codes? Are
   /// they equal or not? This hash value implementation specifically
   /// emphasizes producing different codes for different inputs in order to
   /// be used in canonicalization and memoization. As such, equality is
   /// bitwiseIsEqual, and 0 != -0.
public:
   /// 1.01E-2              4             1       1.01E-2
   void toString(SmallVectorImpl<char> &Str, unsigned FormatPrecision = 0,
                 unsigned FormatMaxPadding = 3) const;
 
   /// If this value has an exact multiplicative inverse, store it in inv and
   /// return true.
   bool getExactInverse(APFloat *inv) const;
 
-  /// \brief Returns the exponent of the internal representation of the APFloat.
+  /// Returns the exponent of the internal representation of the APFloat.
   ///
   /// Because the radix of APFloat is 2, this is equivalent to floor(log2(x)).
   /// For special APFloat values, this returns special error codes:
   ///
   ///   NaN -> \c IEK_NaN
   ///   0   -> \c IEK_Zero
   ///   Inf -> \c IEK_Inf
   ///
   friend int ilogb(const IEEEFloat &Arg);
 
-  /// \brief Returns: X * 2^Exp for integral exponents.
+  /// Returns: X * 2^Exp for integral exponents.
   friend IEEEFloat scalbn(IEEEFloat X, int Exp, roundingMode);
 
   friend IEEEFloat frexp(const IEEEFloat &X, int &Exp, roundingMode);
 
   /// \name Special value setters.
   /// @{
 
   void makeLargest(bool Neg = false);
public:
   opStatus roundToIntegral(roundingMode RM) {
     if (usesLayout<IEEEFloat>(getSemantics()))
       return U.IEEE.roundToIntegral(RM);
     if (usesLayout<DoubleAPFloat>(getSemantics()))
       return U.Double.roundToIntegral(RM);
     llvm_unreachable("Unexpected semantics");
   }
 
+  // TODO: bool parameters are not readable and one source of bugs.
+  //       Do something.
   opStatus next(bool nextDown) {
     if (usesLayout<IEEEFloat>(getSemantics()))
       return U.IEEE.next(nextDown);
     if (usesLayout<DoubleAPFloat>(getSemantics()))
       return U.Double.next(nextDown);
     llvm_unreachable("Unexpected semantics");
   }
 
-  /// \brief Operator+ overload which provides the default
-  /// \c rmNearestTiesToEven rounding mode and *no* error checking.
+  /// Add two APFloats using \c rmNearestTiesToEven rounding mode.
+  /// *No* error checking.
   APFloat operator+(const APFloat &RHS) const {
     APFloat Result(*this);
     (void)Result.add(RHS, rmNearestTiesToEven);
     return Result;
   }
 
-  /// \brief Operator- overload which provides the default
-  /// \c rmNearestTiesToEven rounding mode and *no* error checking.
+  /// Subtract two APFloats using \c rmNearestTiesToEven rounding mode.
+  /// *No* error checking.
   APFloat operator-(const APFloat &RHS) const {
     APFloat Result(*this);
     (void)Result.subtract(RHS, rmNearestTiesToEven);
     return Result;
   }
 
-  /// \brief Operator* overload which provides the default
-  /// \c rmNearestTiesToEven rounding mode and *no* error checking.
+  /// Multiply two APFloats using \c rmNearestTiesToEven rounding mode.
+  /// *No* error checking.
   APFloat operator*(const APFloat &RHS) const {
     APFloat Result(*this);
     (void)Result.multiply(RHS, rmNearestTiesToEven);
     return Result;
   }
 
-  /// \brief Operator/ overload which provides the default
-  /// \c rmNearestTiesToEven rounding mode and *no* error checking.
+  /// Divide the first APFloat by the second, using \c rmNearestTiesToEven
+  /// rounding mode. *No* error checking.
   APFloat operator/(const APFloat &RHS) const {
     APFloat Result(*this);
     (void)Result.divide(RHS, rmNearestTiesToEven);
     return Result;
   }
 
   void changeSign() {
     if (usesLayout<IEEEFloat>(getSemantics())) {
void clearSign() {
     if (isNegative())
       changeSign();
   }
   void copySign(const APFloat &RHS) {
     if (isNegative() != RHS.isNegative())
       changeSign();
   }
 
-  /// \brief A static helper to produce a copy of an APFloat value with its sign
+  /// A static helper to produce a copy of an APFloat value with its sign
   /// copied from some other APFloat.
   static APFloat copySign(APFloat Value, const APFloat &Sign) {
     Value.copySign(Sign);
     return Value;
   }
 
   opStatus convert(const fltSemantics &ToSemantics, roundingMode RM,
                    bool *losesInfo);

 inline APFloat scalbn(APFloat X, int Exp, APFloat::roundingMode RM) {
   if (APFloat::usesLayout<detail::IEEEFloat>(X.getSemantics()))
     return APFloat(scalbn(X.U.IEEE, Exp, RM), X.getSemantics());
   if (APFloat::usesLayout<detail::DoubleAPFloat>(X.getSemantics()))
     return APFloat(scalbn(X.U.Double, Exp, RM), X.getSemantics());
   llvm_unreachable("Unexpected semantics");
 }
 
-/// \brief Equivalent of C standard library function.
+/// Equivalent of C standard library function.
 ///
 /// While the C standard says Exp is an unspecified value for infinity and nan,
 /// this returns INT_MAX for infinities, and INT_MIN for NaNs.
 inline APFloat frexp(const APFloat &X, int &Exp, APFloat::roundingMode RM) {
   if (APFloat::usesLayout<detail::IEEEFloat>(X.getSemantics()))
     return APFloat(frexp(X.U.IEEE, Exp, RM), X.getSemantics());
   if (APFloat::usesLayout<detail::DoubleAPFloat>(X.getSemantics()))
     return APFloat(frexp(X.U.Double, Exp, RM), X.getSemantics());
   llvm_unreachable("Unexpected semantics");
 }
-/// \brief Returns the absolute value of the argument.
+/// Returns the absolute value of the argument.
 inline APFloat abs(APFloat X) {
   X.clearSign();
   return X;
 }
 
 /// Implements IEEE minNum semantics. Returns the smaller of the 2 arguments if
 /// both are not NaN. If either argument is a NaN, returns the other argument.
 LLVM_READONLY

llvm/lib/Support/APFloat.cpp

namespace llvm {
 
   static const fltSemantics semIEEEhalf = {15, -14, 11, 16};
   static const fltSemantics semIEEEsingle = {127, -126, 24, 32};
   static const fltSemantics semIEEEdouble = {1023, -1022, 53, 64};
   static const fltSemantics semIEEEquad = {16383, -16382, 113, 128};
   static const fltSemantics semX87DoubleExtended = {16383, -16382, 64, 80};
   static const fltSemantics semBogus = {0, 0, 0, 0};
 
-  /* The PowerPC format consists of two doubles.  It does not map cleanly
-     onto the usual format above.  It is approximated using twice the
-     mantissa bits.  Note that for exponents near the double minimum,
-     we no longer can represent the full 106 mantissa bits, so those
-     will be treated as denormal numbers.
-
-     FIXME: While this approximation is equivalent to what GCC uses for
-     compile-time arithmetic on PPC double-double numbers, it is not able
-     to represent all possible values held by a PPC double-double number,
-     for example: (long double) 1.0 + (long double) 0x1p-106
-     Should this be replaced by a full emulation of PPC double-double?
+  /* The IBM double-double numbers. It consists of a pair of IEEE 64-bit
+     doubles (Hi, Lo), where |Hi| > |Lo|, and (double)(Hi + Lo) == Hi.
+     The numeric value it's modeling is Hi + Lo. Therefore it has two 53-bit
+     mantissa parts that aren't necessarily adjacent with each other, and an
+     11-bit exponent.
 
      Note: we need to make the value different from semBogus as otherwise
      an unsafe optimization may collapse both values to a single address,
      and we heavily rely on them having distinct addresses.             */
   static const fltSemantics semPPCDoubleDouble = {-1, 0, 0, 0};
 
   /* These are legacy semantics for the fallback, inaccrurate implementation of
      IBM double-double, if the accurate semPPCDoubleDouble doesn't handle the
-     case. It's equivalent to have an IEEE number with consecutive 106 bits of
-     mantissa, and 11 bits of exponent. It's not accurate, becuase the two
-     53-bit mantissa parts don't actually have to be consecutive,
-     e.g. 1 + epsilon.
+     operation. It's equivalent to having an IEEE number with 106 consecutive
+     bits of mantissa and 11 bits of exponent.
+
+     It's not equivalent to IBM double-double. For example, a legit IBM
+     double-double, 1 + epsilon:
+
+       1 + epsilon = 1 + (1 >> 1076)
+
+     is not representable by a consecutive 106 bits of mantissa.
 
      Currently, these semantics are used in the following way:
 
        (IEEEdouble, IEEEdouble) -> (64-bit APInt, 64-bit APInt) ->
        (128-bit APInt) -> semPPCDoubleDoubleLegacy -> IEEE operations
 
      We use bitcastToAPInt() to get the bit representation (in APInt) of the
      underlying IEEEdouble, then use the APInt constructor to construct the
APFloat::opStatus DoubleAPFloat::addImpl(const APFloat &a, const APFloat &aa,
                                          const APFloat &c, const APFloat &cc,
                                          roundingMode RM) {
   int Status = opOK;
   APFloat z = a;
   Status |= z.add(c, RM);
   if (!z.isFinite()) {
     if (!z.isInfinity()) {
       Floats[0] = std::move(z);
-      Floats[1].makeZero(false);
+      Floats[1].makeZero(/* Neg = */ false);
       return (opStatus)Status;
     }
     Status = opOK;
     auto AComparedToC = a.compareAbsoluteValue(c);
     z = cc;
     Status |= z.add(aa, RM);
     if (AComparedToC == APFloat::cmpGreaterThan) {
       // z = cc + aa + c + a;
       Status |= z.add(c, RM);
       Status |= z.add(a, RM);
     } else {
       // z = cc + aa + a + c;
       Status |= z.add(a, RM);
       Status |= z.add(c, RM);
     }
     if (!z.isFinite()) {
       Floats[0] = std::move(z);
-      Floats[1].makeZero(false);
+      Floats[1].makeZero(/* Neg = */ false);
       return (opStatus)Status;
     }
     Floats[0] = z;
     APFloat zz = aa;
     Status |= zz.add(cc, RM);
     if (AComparedToC == APFloat::cmpGreaterThan) {
       // Floats[1] = a - z + c + zz;
       Floats[1] = a;
if (!z.isFinite()) {
     Status |= q.add(z, RM);
     Status |= q.subtract(a, RM);
     q.changeSign();
     Status |= zz.add(q, RM);
     Status |= zz.add(aa, RM);
     Status |= zz.add(cc, RM);
     if (zz.isZero() && !zz.isNegative()) {
       Floats[0] = std::move(z);
-      Floats[1].makeZero(false);
+      Floats[1].makeZero(/* Neg = */ false);
       return opOK;
     }
     Floats[0] = z;
     Status |= Floats[0].add(zz, RM);
     if (!Floats[0].isFinite()) {
-      Floats[1].makeZero(false);
+      Floats[1].makeZero(/* Neg = */ false);
       return (opStatus)Status;
     }
     Floats[1] = std::move(z);
     Status |= Floats[1].subtract(Floats[0], RM);
     Status |= Floats[1].add(zz, RM);
   }
   return (opStatus)Status;
 }
APFloat::opStatus DoubleAPFloat::multiply(const DoubleAPFloat &RHS,
 
   int Status = opOK;
   APFloat A = Floats[0], B = Floats[1], C = RHS.Floats[0], D = RHS.Floats[1];
   // t = a * c
   APFloat T = A;
   Status |= T.multiply(C, RM);
   if (!T.isFiniteNonZero()) {
     Floats[0] = T;
-    Floats[1].makeZero(false);
+    Floats[1].makeZero(/* Neg = */ false);
     return (opStatus)Status;
   }
 
   // tau = fmsub(a, c, t), that is -fmadd(-a, c, t).
   APFloat Tau = A;
   T.changeSign();
   Status |= Tau.fusedMultiplyAdd(C, T, RM);
   T.changeSign();
T.changeSign();
     Status |= Tau.add(V, RM);
   }
   // u = t + tau
   APFloat U = T;
   Status |= U.add(Tau, RM);
 
   Floats[0] = U;
   if (!U.isFinite()) {
-    Floats[1].makeZero(false);
+    Floats[1].makeZero(/* Neg = */ false);
   } else {
     // Floats[1] = (t - u) + tau
     Status |= T.subtract(U, RM);
     Status |= T.add(Tau, RM);
     Floats[1] = T;
   }
   return (opStatus)Status;
 }

 APFloat::fltCategory DoubleAPFloat::getCategory() const {
   return Floats[0].getCategory();
 }
 
 bool DoubleAPFloat::isNegative() const { return Floats[0].isNegative(); }
 
 void DoubleAPFloat::makeInf(bool Neg) {
   Floats[0].makeInf(Neg);
-  Floats[1].makeZero(false);
+  Floats[1].makeZero(/* Neg = */ false);
 }
 
 void DoubleAPFloat::makeZero(bool Neg) {
   Floats[0].makeZero(Neg);
-  Floats[1].makeZero(false);
+  Floats[1].makeZero(/* Neg = */ false);
 }
 
 void DoubleAPFloat::makeLargest(bool Neg) {
   assert(Semantics == &semPPCDoubleDouble && "Unexpected Semantics");
   Floats[0] = APFloat(semIEEEdouble, APInt(64, 0x7fefffffffffffffull));
   Floats[1] = APFloat(semIEEEdouble, APInt(64, 0x7c8ffffffffffffeull));
   if (Neg)
     changeSign();
 }
 
 void DoubleAPFloat::makeSmallest(bool Neg) {
   assert(Semantics == &semPPCDoubleDouble && "Unexpected Semantics");
   Floats[0].makeSmallest(Neg);
-  Floats[1].makeZero(false);
+  Floats[1].makeZero(/* Neg = */ false);
 }
 
 void DoubleAPFloat::makeSmallestNormalized(bool Neg) {
   assert(Semantics == &semPPCDoubleDouble && "Unexpected Semantics");
   Floats[0] = APFloat(semIEEEdouble, APInt(64, 0x0360000000000000ull));
   if (Neg)
     Floats[0].changeSign();
-  Floats[1].makeZero(false);
+  Floats[1].makeZero(/* Neg = */ false);
 }
 
 void DoubleAPFloat::makeNaN(bool SNaN, bool Neg, const APInt *fill) {
   Floats[0].makeNaN(SNaN, Neg, fill);
-  Floats[1].makeZero(false);
+  Floats[1].makeZero(/* Neg = */ false);
 }
 
 APFloat::cmpResult DoubleAPFloat::compare(const DoubleAPFloat &RHS) const {
   auto Result = Floats[0].compare(RHS.Floats[0]);
+  // |Float[0]| > |Float[1]|
   if (Result == APFloat::cmpEqual)
     return Floats[1].compare(RHS.Floats[1]);
   return Result;
 }
 
 bool DoubleAPFloat::bitwiseIsEqual(const DoubleAPFloat &RHS) const {
   return Floats[0].bitwiseIsEqual(RHS.Floats[0]) &&
          Floats[1].bitwiseIsEqual(RHS.Floats[1]);
unsigned int DoubleAPFloat::convertToHexString(char *DST,
   assert(Semantics == &semPPCDoubleDouble && "Unexpected Semantics");
   return APFloat(semPPCDoubleDoubleLegacy, bitcastToAPInt())
       .convertToHexString(DST, HexDigits, UpperCase, RM);
 }
 
 bool DoubleAPFloat::isDenormal() const {
   return getCategory() == fcNormal &&
          (Floats[0].isDenormal() || Floats[1].isDenormal() ||
+          // (double)(Hi + Lo) == Hi defines a normalized number.
           Floats[0].compare(Floats[0] + Floats[1]) != cmpEqual);
 }
 
 bool DoubleAPFloat::isSmallest() const {
   if (getCategory() != fcNormal)
     return false;
   DoubleAPFloat Tmp(*this);
   Tmp.makeSmallest(this->isNegative());