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

[Amy Kwan via Phabricator via llvm-commits]

amyk marked 4 inline comments as done.
amyk 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
----------------
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. 


================
Comment at: compiler-rt/test/builtins/Unit/ppc/floattitf_test.h:16
+struct testCase tests[] = {
+{ INFINITY, 0x1p+127, -0x1p+0 },
+{ -INFINITY, -0x1p+127, 0x0p+0 },
----------------
renenkel wrote:
> nemanjai wrote:
> > I find it really strange that this works. What is `__builtin_inf()` converted to `__int128_t`? Similarly with `__builtin_nan()`.
> 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).
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. 


================
Comment at: compiler-rt/test/builtins/Unit/ppc/floattitf_test.h:16
+struct testCase tests[] = {
+{ INFINITY, 0x1p+127, -0x1p+0 },
+{ -INFINITY, -0x1p+127, 0x0p+0 },
----------------
amyk wrote:
> renenkel wrote:
> > nemanjai wrote:
> > > I find it really strange that this works. What is `__builtin_inf()` converted to `__int128_t`? Similarly with `__builtin_nan()`.
> > 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).
> 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. 
Yes, you are correct. I will remove them as I see they don't quite make sense as inputs for the conversion. 


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

https://reviews.llvm.org/D54313