[PATCH] D54313: [compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC
Robert Enenkel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Dec 19 20:39:46 PST 2018
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
More information about the llvm-commits
mailing list