[APFloat] Fix comments. NFC.

[timshen]

Summary

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

[jlebar]

llvm/include/llvm/ADT/APFloat.h

1054 ↗

(On Diff #85672)

s/one/a/

1055 ↗

(On Diff #85672)

Nit, this indentation seems noncanonical.

1064 ↗

(On Diff #85672)

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

1065 ↗

(On Diff #85672)

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

llvm/lib/Support/APFloat.cpp

69 ↗

(On Diff #85672)

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

70 ↗

(On Diff #85672)

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

72 ↗

(On Diff #85672)

adjacent to

82 ↗

(On Diff #85672)

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

4243 ↗

(On Diff #85672)

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 ↗

(On Diff #85672)

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

[timshen]

Tweak wording.

llvm/include/llvm/ADT/APFloat.h

1065 ↗

(On Diff #85672)

Removed. I don't like it either.

llvm/lib/Support/APFloat.cpp

69 ↗

(On Diff #85672)

Tweaked a bit.

70 ↗

(On Diff #85672)

Added ", and if normalized, ..."

[jlebar]

lgtm!

llvm/lib/Support/APFloat.cpp

70 ↗

(On Diff #85831)

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. :)

4341 ↗

(On Diff #85831)

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 ↗

(On Diff #85831)

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

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

llvm/trunk/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 a 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, rounding ties to the nearest even.
+  /// 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, rounding ties to the nearest even.
+  /// 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, rounding ties to the nearest even.
+  /// 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, rounding ties to the nearest even.
+  /// 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;
 }
 
 /// \brief Returns the negated value of the argument.
 inline APFloat neg(APFloat X) {
   X.changeSign();

llvm/trunk/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 semantics. Such a number consists of a pair of IEEE
+     64-bit doubles (Hi, Lo), where |Hi| > |Lo|, and if normal,
+     (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
+     to each other, and two 11-bit exponents.
 
      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 consecutive 106
+     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
+       semPPCDoubleDouble -> (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
      legacy IEEE float.
 
      TODO: Implement all operations in semPPCDoubleDouble, and delete these
      semantics.  */
   static const fltSemantics semPPCDoubleDoubleLegacy = {1023, -1022 + 53,
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 normal 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());