[PATCH] D27872: [APFloat] Switch from (PPCDoubleDoubleImpl, IEEEdouble) layout to (IEEEdouble, IEEEdouble)

[Justin Lebar via Phabricator via cfe-commits]

jlebar added a comment.

Eric asked me to do a review of this.  I notice now it was submitted while I was doing the review.  Oh well.  Here are the comments I had.



================
Comment at: llvm/include/llvm/ADT/APFloat.h:1054
 
+  opStatus next(bool nextDown) {
+    if (usesLayout<IEEEFloat>(getSemantics()))
----------------
FWIW, see my screed on this: http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html


================
Comment at: llvm/include/llvm/ADT/APFloat.h:1062
+
+  /// \brief Operator+ overload which provides the default
+  /// \c rmNearestTiesToEven rounding mode and *no* error checking.
----------------
We have autobrief, which I do not understand, but I believe renders "\brief" unnecessary?


================
Comment at: llvm/include/llvm/ADT/APFloat.h:1062
+
+  /// \brief Operator+ overload which provides the default
+  /// \c rmNearestTiesToEven rounding mode and *no* error checking.
----------------
jlebar wrote:
> We have autobrief, which I do not understand, but I believe renders "\brief" unnecessary?
Nit, Since "operator" is a keyword, it should be capitalized as in the C++


================
Comment at: llvm/include/llvm/ADT/APFloat.h:1062
+
+  /// \brief Operator+ overload which provides the default
+  /// \c rmNearestTiesToEven rounding mode and *no* error checking.
----------------
jlebar wrote:
> jlebar wrote:
> > We have autobrief, which I do not understand, but I believe renders "\brief" unnecessary?
> Nit, Since "operator" is a keyword, it should be capitalized as in the C++
Nit, It's clear from the code that this is an operator+ overload.  Perhaps all we need to say is

> Add two APFloats using rmNearestTiesToEven semantics.  No error checking.


================
Comment at: llvm/lib/Support/APFloat.cpp:88
+     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
----------------
s/case/operation/?


================
Comment at: llvm/lib/Support/APFloat.cpp:88
+     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
----------------
jlebar wrote:
> s/case/operation/?
s/have/having/


================
Comment at: llvm/lib/Support/APFloat.cpp:88
+     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
----------------
jlebar wrote:
> jlebar wrote:
> > s/case/operation/?
> s/have/having/
s/consecutive 106 bits/106 consecutive bits/


================
Comment at: llvm/lib/Support/APFloat.cpp:89
+     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,
----------------
No comma before "and".  (Comma before "and" is usually reserved for the case when "and" joins two "independent clauses" -- things which could themselves be complete sentences.)


================
Comment at: llvm/lib/Support/APFloat.cpp:89
+     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,
----------------
jlebar wrote:
> No comma before "and".  (Comma before "and" is usually reserved for the case when "and" joins two "independent clauses" -- things which could themselves be complete sentences.)
Suggest

> It's not equivalent to the true PPC double-double semantics, because in reality, the two 53-bit mantissa parts ...


================
Comment at: llvm/lib/Support/APFloat.cpp:91
+     53-bit mantissa parts don't actually have to be consecutive,
+     e.g. 1 + epsilon.
 
----------------
Can we rephrase "the two 53-bit mantissa parts don't actually have to be consecutive, e.g. 1 + epsilon."?  I don't understand what this means.


================
Comment at: llvm/lib/Support/APFloat.cpp:4135
+  Floats[0].makeZero(Neg);
+  Floats[1].makeZero(false);
+}
----------------
Nit: `makeZero(/* Neg = */ false)`?  (See aforementioned screed.  :)


================
Comment at: llvm/lib/Support/APFloat.cpp:4168
+  if (Result == APFloat::cmpEqual)
+    return Floats[1].compare(RHS.Floats[1]);
+  return Result;
----------------
Is it worth adding a comment here that this is correct because in a double-double (x, y), we always have x >= y?

At least, I assume that's why this is correct.  :)


================
Comment at: llvm/lib/Support/APFloat.cpp:4179
+  if (Arg.Floats)
+    return hash_combine(hash_value(Arg.Floats[0]), hash_value(Arg.Floats[1]));
+  return hash_combine(Arg.Semantics);
----------------
Nit, do you want to include the semantics?  I mean, I guess it doesn't really matter.


================
Comment at: llvm/lib/Support/APFloat.cpp:4263
+         (Floats[0].isDenormal() || Floats[1].isDenormal() ||
+          Floats[0].compare(Floats[0] + Floats[1]) != cmpEqual);
+}
----------------
This shouldn't be `== cmpEqual` or something?  Otherwise this makes no sense to me; perhaps we could add a comment?


================
Comment at: llvm/unittests/ADT/APFloatTest.cpp:3511
+      // (1 + 0) = (1 + 0)
+      std::make_tuple(0x3ff0000000000000ull, 0, 0x3ff0000000000000ull, 0, true),
+      // (1 + 0) != (1.00...1 + 0)
----------------
Can you use {} instead of make_tuple?  https://godbolt.org/g/1Y1P0F


================
Comment at: llvm/unittests/ADT/APFloatTest.cpp:3534
                    Op2[1])
                .str();
   }
----------------
Honestly I am not sure this is an improvement over writing it out by hand:

EXPECT_TRUE(APFloat(APFloat::PPCDoubleDouble(), APInt(128, 2, {0xblah, 0xblah})).bitwiseIsEqual(...)))

(You should be able to pass an initializer_list to the constructor, no problem, afaict.)  Up to you though.


Repository:
  rL LLVM

https://reviews.llvm.org/D27872