[PATCH] D54313: [compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC

[Robert Enenkel via Phabricator via llvm-commits]

renenkel added inline comments.


================
Comment at: compiler-rt/lib/builtins/ppc/floattitf.c:21
+
+long double __floattitf (__int128_t arg) {
+  /* 128 bit integers can be thought of as having a high and a low
----------------
amyk wrote:
> nemanjai wrote:
> > Just out of curiosity, where does `__int128_t` come from if we don't include a header that defines it? Does this compile with both gcc and clang?
> The `__int128_t` and `__uint128_t` types are defined within gcc (cc1), not in a header I believe. They are available in both gcc and clang. There does not seem to be anything in the gcc documentation regarding these types, but they seem equivalent to `__int128` and `unsigned __int128` (https://gcc.gnu.org/ml/libstdc++/2011-09/msg00068.html).
> 
>  I would have used `__int128` and `unsigned __int128`, however compiling with them gave me warnings in LLVM saying that they were unsupported. 
"This file implements converting an unsigned 128 bit integer to a 128bit IBM/PowerPC long double (double-double) value."  No, this is the signed conversion.

What's in DD.h?  Is it needed?  If not, remove it.

The variable hiPart.  HIgh part of what?  How about arg_hiPart?

The variables hiDouble and loDouble.  These names are misleading.  They're not doubles.  They're arg_hiPart and arg_loPart converted to long double.   How about LD_arg_loPart and LD_arg_hiPart?

The choice of name of the constant eachDWordSize is misleading, as is the comment, "The size of each high and low part of the 128 bit integer."  It's not a word size but rather two the power of 64, the value you multiply by to shift by the word size.  A better name would be simply two64.  However, why initialize a double constant with this value, when in the next line you cast it into a long double? 

Here is what I think the code should look like.  (After changing it, to be safe, you should run Masoud's full-range test harness again.)

```
//===-- lib/builtins/ppc/floattitf.c - Convert int128->long double -*-C -*-===//
//
//                     The LLVM Compiler Infrastructure
//
// This file is dual licensed under the MIT and the University of Illinois Open
// Source Licenses. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//
//
// This file implements conversion of a signed 128 bit integer to a 128bit IBM/
// PowerPC long double (double-double) value.
//
//===----------------------------------------------------------------------===//

/* Conversions from signed and unsigned 64-bit int to long double */
long double __floatditf (int64_t);
long double __floatunditf (uint64_t);

/* Convert signed 128-bit int to long double.  This uses the following
 * property:  Let hi and lo be 64-bits each, and let signed_val_k() and
 * unsigned_val_k()be the value of the argument interpreted as a signed
 * or unsigned k-bit integer, respectively.  Then,
 * signed_val_128(hi,lo) = signed_val_64(hi) * 2^64 + unsigned_val_64(lo)
 * = (long double)hi * 2^64 + (long double)lo,
 * where (long double)hi and (long double)lo are signed and
 * unsigned 64-bit int to long double conversions, respectively.
*/

long double __floattitf (__int128_t arg) {
  /* Split the int128 argument into high and low 64-bit parts. */
  int64_t arg_hiPart = (int64_t)(arg >> 64);
  int64_t arg_loPart = (int64_t) arg;

  /* Convert each 64-bit part into long double. The high part
   * must be a signed conversion and the low part an unsigned
   * conversion to produce the correct result. */
  long double LD_arg_hiPart = __floatditf(arg_hiPart);
  long double LD_arg_loPart = __floatunditf(arg_loPart);

  /* The low bit of arg_hiPart corresponds to the 2^64 bit in arg.
   * Multiply the high part by 2^64 to undo the right shift by
   * 64-bits done in the splitting, and add to the low part to
   * obtain the final result.
  static const double two64 = 0x1.0p64;  // 2^64
  return (LD_arg_hiPart * two64 + LD_arg_loPart);
}
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54313/new/

https://reviews.llvm.org/D54313