Page MenuHomePhabricator

[compiler-rt] [builtins] Implement __floattitf() & __floatuntitf()
ClosedPublic

Authored by mgorny on Dec 18 2016, 3:38 AM.

Details

Summary

Implement the missing floattitf() and floatuntitf() functions, to
convert 128-bit (unsigned) integers to quad-precision floating-point
types. This is needed e.g. on AArch64 where 'long double' is
a quad-precision type.

The code is based on the existing code for floattixf()
and
floatuntixf(), updated to account for different bit field lengths
of quad-precision float. The tests are also copied, with the rounding
tests adjusted for longer significand.

(tested on AArch64)

Diff Detail

Event Timeline

mgorny updated this revision to Diff 81876.Dec 18 2016, 3:38 AM
mgorny retitled this revision from to [compiler-rt] [builtins] Implement __floattitf() & __floatuntitf().
mgorny updated this object.
mgorny added reviewers: sdmitrouk, rengolin, zatrazz.
mgorny added a subscriber: cfe-commits.
weimingz edited edge metadata.Dec 20 2016, 10:35 PM

The code looks OK except for the formatting. Since the logic is almost the same as floattixf / floatuntixf except that some constants are different, I'm wondering if it makes sense to reuse the existing code? For example, make existing code as floattixf_impl.inc

lib/builtins/floattitf.c
33

Please put "{" on the same line like __floattitf(ti_int a) {
Same for other occurrences below

mgorny updated this revision to Diff 82235.Dec 21 2016, 7:23 AM
mgorny edited edge metadata.
mgorny marked an inline comment as done.

I think I've shifted all of {/} to be in line with keywords.

As for the splitting the code, you're probably right. However, I'd rather do that in a separate patch (I'll add it to my TODO, I promise).

I think that most of those files reuse a similar code -- I think it's for the case when the integer has greater range than the float. It would certainly also make sense to try to commonize the code used for signed and unsigned. However, I'm a little worried that it might make the result unreadable.

Thanks for the formatting. Regarding the refactoring, Regarding factoring out common code, I think it's OK to do it in a follow-up patch. For readability, it should be OK. For example, fp_add_impl.inc is used by float, double and long double.

The patch looks good to me.

rengolin accepted this revision.Dec 22 2016, 9:35 AM
rengolin edited edge metadata.

My floating-point-fu is lacking, so I'll trust you guys that this makes sense. :)

I didn't see anything egregious, so LGTM with the nit inline.

cheers,
--renato

lib/builtins/floatuntitf.c
74

nit: an hexadecimal pattern here would be clearer. same above.

This revision is now accepted and ready to land.Dec 22 2016, 9:35 AM
mgorny added inline comments.Dec 22 2016, 10:04 AM
lib/builtins/floatuntitf.c
74

Do you mean something like: (foo << 48) & 0xffff....?

rengolin added inline comments.Dec 22 2016, 10:17 AM
lib/builtins/floatuntitf.c
74

No, just the 16383 to 0x3FFF

scanon added a subscriber: scanon.Dec 22 2016, 10:20 AM

Please s/mantissa/significand/. I know that "mantissa" is used in a number of places in llvm sources, but it's incorrect terminology. Significand is the IEEE-754 nomenclature, which any new work should follow.

rengolin added inline comments.Dec 22 2016, 10:20 AM
lib/builtins/floatuntitf.c
74

Though, now I think your proposal may be better. :)

Whatever works, I'm not too concerned about that.

scanon added inline comments.Dec 22 2016, 10:36 AM
lib/builtins/floattitf.c
66

Strictly speaking there's no need to adjust a here. If we rounded up into a new binade, then a is necessarily 0b1000...0, and the leading 1 bit will get killed by the mask when we assemble fb.u.high.all regardless of its position. Same comment applies to floatuntitf.

mgorny added inline comments.Dec 22 2016, 10:38 AM
lib/builtins/floatuntitf.c
74

Well, I used the decimal form because that's how the IEEE 754 standard specifies it, so I guessed the correlation would be clearer that way.

scanon added inline comments.Dec 22 2016, 10:41 AM
lib/builtins/floatuntitf.c
74

FWIW, I think both are pretty obvious, but if you want to be totally explicit:

const int bias = 0x3fff;
... (e + bias) ...
mgorny added inline comments.Dec 23 2016, 1:01 AM
lib/builtins/floattitf.c
66

I'm sorry but I don't feel confident changing that. AFAIU if the LDBL_MANT_DIG+1 bit is set, this code shifts it lower, so it won't actually be killed by the mask.

scanon added inline comments.Dec 23 2016, 4:51 AM
lib/builtins/floattitf.c
66

In binary128, as in all IEEE 754 binary interchange format encodings, the leading bit of the significand is implicit. The only way to end up in this code path is 0b111...1 rounding up to 0b100...00, meaning that the significand is 1.0, which is stored as all-zeros (i.e. the leading bit is necessarily masked).

To be more explicit, LDBL_MANT_DIG is 113. If this shift happens, after the shift bit 112 is set, and bits 111:0 are zero. The mask ((a >> 64) & 0x0000ffffffffffffLL) discards bit 112 (= 64 + 48).

scanon requested changes to this revision.Dec 23 2016, 4:54 AM
scanon added a reviewer: scanon.

I don't particularly care about the shift, since the code is completely equivalent either way, though it would be nice to clean up. However, please replace "mantissa" with "significand" before committing.

This revision now requires changes to proceed.Dec 23 2016, 4:54 AM
mgorny updated this revision to Diff 82414.Dec 23 2016, 9:33 AM
mgorny updated this object.
mgorny edited edge metadata.

Changed mantissa -> significand.

mgorny added inline comments.Jan 6 2017, 7:28 AM
lib/builtins/floattitf.c
66

Well, I've tried removing this and it causes one of the tests to fail:

error in __floatuntitf(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF) = 0X1P+127, expected 0X1P+128

scanon added inline comments.Jan 6 2017, 7:54 AM
lib/builtins/floattitf.c
66

This sounds like you removed the exponent adjustment (++e) as well as (or instead of) the shift (a >>= 1). Without seeing the change, I can't be certain, of course.

mgorny added inline comments.Jan 6 2017, 9:01 AM
lib/builtins/floattitf.c
66

Yes, I did that. Now that you state that plainly, I finally understand what you meant ;-). However, if I were to do that, I would rather to do that separately, either in all relevant files or post unifying the code into one logical .inc. This would keep the history of changes cleaner.

scanon accepted this revision.Jan 6 2017, 10:00 AM
scanon edited edge metadata.

OK, I'm satisfied with that.

This revision is now accepted and ready to land.Jan 6 2017, 10:00 AM
This revision was automatically updated to reflect the committed changes.