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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
