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

[Tim Shen via Phabricator via cfe-commits]

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