This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][builtins][PowerPC] Add floattitf builtin compiler-rt method support for PowerPC
ClosedPublic

Authored by amyk on Nov 9 2018, 6:59 AM.

Details

Summary

This patch implements the long double floattitf (int128_t) method for PowerPC — specifically to convert a 128 bit integer into a long double (IBM double-double).

The algorithm used in this conversion method from int128_t to long double utilizes the long double floatditf (int64_t) and long double floatunditf (uint64_t) methods, which are already implemented under lib/builtins/ppc/.

This method can be invoked by linking against compiler-rt instead of libgcc, via the -rtlib=compiler-rt command line option supplied to clang.

Diff Detail

Event Timeline

amyk created this revision.Nov 9 2018, 6:59 AM

The summary doesn't match the code. The code floattitf.c converts from 128-bit int to long double, but the summary says it goes the opposite direction.

amyk edited the summary of this revision. (Show Details)Nov 9 2018, 8:53 AM

@renenkel Yes, thank you for pointing that out. I accidentally mixed that up, my apologies. The summary has been corrected to reflect the correct behaviour.

I'll let @renenkel approve or request changes to this as he understands the math much better than I do, but I do want an investigation into how the INFINITY/QNAN tests work along with a comment explaining that. Particularly since I would have expected that every __int128_t value is a number (i.e. not a NAN) and none of them produce an infinity.

compiler-rt/lib/builtins/ppc/floattitf.c
21

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?

compiler-rt/test/builtins/Unit/ppc/floattitf_test.h
16

I find it really strange that this works. What is __builtin_inf() converted to __int128_t? Similarly with __builtin_nan().

renenkel requested changes to this revision.Dec 6 2018, 2:19 PM

I think the use of the DD.h include file makes the code confusing, for several reasons: 1. It requires you to look at two files; 2. An additional assignment is required to interpret the component doubles of the long double as 64-bit ints. I suggest you retain the original form of the union, for example:

#define HI 0
#define LO 1
// Use enum HILO {HI=0, LO=1}; if LLVM style discourages #define

union {long double ld; double d[2]; uint64_t ui[2];} uld;

uld.ld = ld;
// Now you can access any of them easily...
uld.d[HI]
uld.d[LO]
uld.ui[HI]
uld.ui{LO]

This revision now requires changes to proceed.Dec 6 2018, 2:19 PM

Regarding Nemanjai's comment, "I would have expected that every __int128_t value is a number (i.e. not a NAN) and none of them produce an infinity." Correct. No 128-bit int converted to a long double should produce NaN or Inf. However, we have to decide what should happen when a long double NaN or Inf gets converted to a 128-bit int. It seems reasonable that +-Inf gets converted to the largest/smallest int128 of the same sign. But there is not really any natural value for NaN. Unless there is a standard governing this, I would suggest it should be undefined. We've chosen to retain the same bit pattern of the long double NaN in the converted int128. The gcc conversion produces a strange arbitrary number 0x80...080...080...080...0. I'm not sure what the value would be in matching this, but it would be possible with a special case, should that be deemed necessary.

renenkel added a comment.EditedDec 6 2018, 2:38 PM

Masoud wrote a test program that exercises the conversions over the full range of exponents and compares the results to the gcc conversion. My understanding is that the results matched except for the long double NaN (which is arbitrary), and some long doubles with gaps between the high and low double, for which it was possible to determine that the gcc result was wrong (Masoud, please confirm?)

renenkel added a comment.EditedDec 6 2018, 2:56 PM
This comment has been deleted.
compiler-rt/test/builtins/Unit/ppc/floattitf_test.h
16

Do I understand correctly that testCase.input128 is the 128-bit int input to the conversion, and testCase.hi and lo are the hi and lo doubles of the long double that should be produced by the conversion?

In this case, as Nemanjai points out, INFINITY and QNAN don't make sense as inputs to the int128->long double conversion. Those four lines should be removed (in which case the corresponding #defines aren't needed and can be removed too).

amyk marked 4 inline comments as done.Dec 12 2018, 1:27 PM
amyk added inline comments.
compiler-rt/lib/builtins/ppc/floattitf.c
21

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.

compiler-rt/test/builtins/Unit/ppc/floattitf_test.h
16

Yeah, that is true. I originally added these since I based my tests off of existing compiler-rt PPC builtins tests, and they included testing for inf/nan in this manner. I went back to see the high and low parts of both of those functions and they did not really correspond to the values of infinity and NaN that Masoud/Robert previously talked about in our conversations. As per Robert's suggestion, I will remove these since they do not really make sense to include.

16

Yes, you are correct. I will remove them as I see they don't quite make sense as inputs for the conversion.

amyk updated this revision to Diff 178314.Dec 14 2018, 4:13 PM
amyk marked an inline comment as done.

Updated this patch to reflect:

  • Changed variable names;
  • Removed test cases with INFINITY and NAN
  • Added the long double union structure (containing high and low double portions, and their corresponding bit patterns) to be used in the testing file (to remove the dependency of looking at two files for the implementation of the long double)
renenkel added inline comments.Dec 19 2018, 8:39 PM
compiler-rt/lib/builtins/ppc/floattitf.c
21

"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);
}
renenkel requested changes to this revision.Dec 19 2018, 8:45 PM

In floattitf_test.c,

typedef union {
  long double ld;
  double doublePairs[2]; /* [0] is the high double, [1] is the low double. */
  unsigned long long doubleBitPatterns[2];
} ldStructure;

is confusing. Clearer would be:

typedef union {
  long double ld;
  double d[2]; /* [0] is the high double, [1] is the low double. */
  unsigned long long ull[2]; /* high and low doubles as 64-bit ints */
} ldStructure;

is con

This revision now requires changes to proceed.Dec 19 2018, 8:45 PM
amyk updated this revision to Diff 179099.EditedDec 20 2018, 10:39 AM

Updated patch to address @renenkel's review :

  • Renamed union type to represent the long double
  • Updated comments
  • Updated variable names
  • Updated algorithm to be more concise and succinct

This revision has been tested using Masoud's test harness. There are no changes or errors uncovered in the test harness with this updated revision.

amyk updated this revision to Diff 179139.Dec 20 2018, 1:10 PM

Updated this patch with:

  • initialization of int128 macro to simplify test cases
nemanjai accepted this revision.Dec 24 2018, 10:11 AM

The remaining comments from me are minor. I am OK with this going in if Robert is.

compiler-rt/lib/builtins/ppc/floattitf.c
36

uint64_t vs int64_t?
ArgLoPart?

42

I am not sure if any compiler would emit warnings in pedantic mode for a signed argument being passed to a function that takes an unsigned parameter. But I also don't see a compelling need to have both of these signed. The algorithm described states that the low part is treated as unsigned. Why not just declare it as uint64_t as well?

Also, I know that there isn't as much adherence to general LLVM naming conventions in compiler-rt currently, but I don't see why we wouldn't adhere to them on new code.
Seems that these should be along the lines of ConvertedHiPart, etc.

48

I don't think we should add this temporary.

amyk marked 5 inline comments as done.Jan 3 2019, 5:22 AM
amyk added inline comments.
compiler-rt/lib/builtins/ppc/floattitf.c
42

Yeah. that is a good point. I will change the low part to unsigned, and also rename the variables a little to adhere to LLVM naming conventions.

48

I will remove it.

amyk updated this revision to Diff 180040.Jan 3 2019, 5:25 AM
amyk marked 2 inline comments as done.

Updated patch to address:

  • renamed variables for names that adhere more to LLVM naming conventions
  • changed the low part of the function argument to unsigned, as it will later be passed into a function that expects an unsigned argument
  • Removed a temporary that represented 2^64.

I'm okay with the style of the code now. Did you run Masoud's test program after making your changes? If not, you need to do that.

renenkel accepted this revision.Jan 9 2019, 1:30 PM

Amy ran Masoud's tests, so I approve.

This revision is now accepted and ready to land.Jan 9 2019, 1:30 PM

Amy, can we get this committed?

amyk closed this revision.Jan 15 2019, 3:58 AM

Amy, can we get this committed?

I didn't realize this, but I have already committed this builtin along with __fixunsuftsti recently, however I am not sure why this revision is not closed. The other one was closed when I committed it and supplied the revision number, but for some reason, this one was not closed. I will close this now as this was been committed.