This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by amyk on Nov 26 2018, 11:52 AM.

Details

Summary

This patch implements the __uint128_t __fixunstfti (long double) method for PowerPC -- specifically to convert a long double (IBM double-double) to an unsigned 128 bit integer.

The general approach is to convert the high and low doubles of the long double and add them together; this is done in the simplest case when the high and low doubles fit within 64 bits. However, additional adjustments and scaling is performed when the high or low double does not fit within a 64 bit integer. I was assisted by @masoud.ataei and @renenkel in constructing the algorithm, various implementation details and methods of testing for correctness in regards to this function.

To invoke this method, one can do so by linking against compiler-rt instead of libgcc, via the --rtlib=compiler-rt command line option supplied to clang during the compilation step.

Diff Detail

Repository
rL LLVM

Event Timeline

amyk created this revision.Nov 26 2018, 11:52 AM
amyk edited the summary of this revision. (Show Details)Nov 26 2018, 11:56 AM
nemanjai added inline comments.Dec 6 2018, 6:04 AM
compiler-rt/lib/builtins/ppc/fixunstfti.c
20 ↗(On Diff #175303)

Is this the common practice in compiler-rt for detecting NaN's vs something like isnan()?

27 ↗(On Diff #175303)

Do we have a different coding guideline for compiler-rt in terms of naming variables?

42 ↗(On Diff #175303)

s/minusing/subtracting

60 ↗(On Diff #175303)

Please state what the edge cases are for both of the conditions. It is not at all clear to the reader why we will saturate the value when the exponent in the high double is above 128 and the sign bit isn't set. Similarly for the opposite case where we seem to produce 1.

75 ↗(On Diff #175303)

This code seems like it's just a repeat of what we do for the bits in the high double above. Can we common this up into a macro?

compiler-rt/test/builtins/Unit/ppc/fixunstfti_test.h
14 ↗(On Diff #175303)

It might be more readable to define a macro such as:
#define INIT_U128(HI, LO) (((__uint128_t) (HI) << 64) | (LO))

And use that to initialize the 128-bit int values.

amyk marked 6 inline comments as done.Dec 13 2018, 5:20 AM
amyk added inline comments.
compiler-rt/lib/builtins/ppc/fixunstfti.c
20 ↗(On Diff #175303)

That is true, there is a crt_isnan() that can be used within compiler-rt, so I will use that.

42 ↗(On Diff #175303)

Comment has been adjusted.

compiler-rt/test/builtins/Unit/ppc/fixunstfti_test.h
14 ↗(On Diff #175303)

That is a good point; I agree with your suggestion. I will utilize a macro since it also enhances readability.

masoud.ataei added inline comments.Dec 14 2018, 12:58 PM
compiler-rt/lib/builtins/ppc/fixunstfti.c
23 ↗(On Diff #175303)

Here we are returning NaN bit pattern for NaN input. It was originally my suggestion but I still don't know if it is good idea. Maybe it is better to remove this if statement and let the rest of algorithm create an output. In fact, any output is junk. (Since there is no NaN in int.)

60 ↗(On Diff #175303)

when the exponent is >=128 it is overflow. Since this is the conversion to unsigned int128, it will return the max integer when input is positive and it return 1 (as gcc does it).

amyk updated this revision to Diff 178580.Dec 17 2018, 7:22 PM
amyk marked 5 inline comments as done.

Updated this patch with:

  • Renamed variables
  • Utilized compiler-rt specific functions (like NaN)
  • Added the long double union structure in the same file as implementation (containing high and low doubles, high and low bit patterns)
amyk updated this revision to Diff 178704.Dec 18 2018, 9:19 AM
This comment was removed by amyk.
amyk updated this revision to Diff 178706.EditedDec 18 2018, 9:23 AM

Previously uploaded the wrong diff by accident.

Updated revision to fully address:

  • Renamed variables
  • Added macro to initialize int128
  • Macro to group up common code that is used in two cases (high and low cases)
  • Added the long double union structure in the same file as implementation (containing high and low doubles, high and low bit patterns)
amyk marked 2 inline comments as done.Dec 18 2018, 9:26 AM

FWIW once nemanja and Masoud are happy then this can go in.

renenkel requested changes to this revision.EditedDec 19 2018, 9:12 PM

In floattitf_test.c and fixunstfti.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. Also, it's a union, not a structure. 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 */
} ldUnion;

Make the corresponding changes in the summary section, since DD.h is not being used any more.

This revision now requires changes to proceed.Dec 19 2018, 9:12 PM

In the summary, we're not using DD.h any more, so that struct code should be changed to the new version.

compiler-rt/lib/builtins/ppc/fixunstfti.c
23 ↗(On Diff #175303)

I agree that the integer conversion of a NaN should be undefined. If we want to match the meaningless number that gcc produces, we can do that. Or we can do what Masoud did, which was to return the NaN bit pattern. Or we can return anything else. Does the C standard say anything about what such a conversion should produce? Hubert Tong might know that.

compiler-rt/test/builtins/Unit/ppc/fixunstfti_test.h
14 ↗(On Diff #175303)

I agree.

renenkel added a comment.EditedDec 19 2018, 10:18 PM

I don't understand why you need the if statement in SCALE_DOUBLE. Since you are converting to unsigned int128, I don't know why you cast to (unsigned long long) in one case and (long long) in the other. Check this with Masoud, but I think you can do both the same way.
SCALE_DOUBLE deserves more explanatory comments.
doubleType is confusing. It's not a type. It's either 0 or 1 for the high of low double.
ldStructure doesn't appear in the macro arguments, but is assumed, which is confusing.
I've also changed the union fields according to my other comment.
The name SCALE_DOUBLE is misleading, since, although it does scale, it really converts a double to 64-bit int.
Operand order is inconsistent, the output is one of the middle arguments.
How about this?

// Scale selected double of input ld union to allow conversion to 64-bit int.
// hilo selects the desired double:  0=hi double, 1=lo double.
// exponent = unbiased exponent of selected double.
// result is ld.d[hilo] scaled to have exponent 53, converted to 64-bit int, and shifted to undo scaling.
#define DOUBLE_TO_INT64 (ld, hilo, exponent, result)                             \
  shift = exponent - 54;                                                       \
  ld.ull[hilo] &= 0x800FFFFFFFFFFFFFll;           \
  ld.ull[hilo] |= 0x4350000000000000ll;           \                                                \
  result = (unsigned long long)ld.d[hilo];          \                                                                      \
  result <<= shift;
amyk marked an inline comment as done.Dec 20 2018, 11:59 AM
amyk added inline comments.
compiler-rt/lib/builtins/ppc/fixunstfti.c
75 ↗(On Diff #175303)

I have considered putting the common code into a macro and have previously done so. However, doing so still requires me to place an if-condition inside since the high and low double cases are not exactly identical. Due to this, discussions with Robert has lead to letting the duplicated code stay as it is since the two code segments are not entirely the same.

amyk updated this revision to Diff 179115.Dec 20 2018, 12:05 PM
amyk edited the summary of this revision. (Show Details)

Updated this patch to address:

  • Updated comments that are more descriptive
  • Renamed variables
  • Removed the macro originally used to group up common code. Since the code that seems duplicated is not entirely identical, the code will remain duplicated as utilizing a macro still requires an if-condition to control the conversion depending if we are converting and high double or a low double

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.

If you haven't already, need to run Masoud's test program after making your changes.

compiler-rt/test/builtins/Unit/ppc/fixunstfti_test.h
14 ↗(On Diff #175303)

Minor point - there are places in the conversion routine where INIT_U128 could also be used. (Not essential to change this if you've already run Masoud's tests though.)

renenkel accepted this revision.Jan 9 2019, 1:30 PM
This revision is now accepted and ready to land.Jan 9 2019, 1:30 PM
This revision was automatically updated to reflect the committed changes.