timshen marked 13 inline comments as done.
timshen added inline comments.
================
Comment at: llvm/include/llvm/ADT/APFloat.h:1054
+ opStatus next(bool nextDown) {
+ if (usesLayout<IEEEFloat>(getSemantics()))
----------------
jlebar wrote:
> FWIW, see my screed on this:
> http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html
I totally agree, but I'm not going to change it right now, so I added a TODO.
================
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:
> 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 ...
I re-worked on the comments to make it readable by a programmer with no IBM
double-double knowledge.
================
Comment at: llvm/lib/Support/APFloat.cpp:4168
+ if (Result == APFloat::cmpEqual)
+ return Floats[1].compare(RHS.Floats[1]);
+ return Result;
----------------
jlebar wrote:
> 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. :)
I think you mean |x| > |y|. Now it's mentioned in the semPPCDoubleDouble
documentation.
================
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);
----------------
jlebar wrote:
> Nit, do you want to include the semantics? I mean, I guess it doesn't really
> matter.
It's more like a defensive style, just in case DoubleAPFloat supports more than
just two doubles.
================
Comment at: llvm/lib/Support/APFloat.cpp:4263
+ (Floats[0].isDenormal() || Floats[1].isDenormal() ||
+ Floats[0].compare(Floats[0] + Floats[1]) != cmpEqual);
+}
----------------
jlebar wrote:
> This shouldn't be `== cmpEqual` or something? Otherwise this makes no sense
> to me; perhaps we could add a comment?
It's by IBM double-double's definition, as previously commented.
================
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)
----------------
jlebar wrote:
> Can you use {} instead of make_tuple? https://godbolt.org/g/1Y1P0F
Nope, GCC 4.8 will puke. lul.
================
Comment at: llvm/unittests/ADT/APFloatTest.cpp:3534
Op2[1])
.str();
}
----------------
jlebar wrote:
> 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.
It's more of being consistent with other tests. I might be too lazy to change,
unless you insist. :)
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
