Add _Float128 as alias to __float128 to enable compilations on Fedora27/glibc2-26

[mibintc]

Summary

Fedora27 is using a new version of glibc that refers to the _Float128 type. This patch adds that name as an alias to float128. I also added some predefined macro values for the digits, mantissa, epilon, etc (FloatMacros). For the test case, I copied an existing float128 test. This functionality needs work long term, but it should be sufficient to tread water for a while. At Intel we have test servers running our LLVM compiler with various open source workloads, the server has been upgraded to Fedora27 so many workloads are failing due to _Float128. What do you think?

[jlebar]

LGTM for the CUDA test changes.

[rsmith]

Per https://gcc.gnu.org/onlinedocs/gcc/Floating-Types.html, this doesn't appear to be correct: _Float128 is only *sometimes* the same type as __float128.

[efriedma]

_Float128 is only *sometimes* the same type as __float128.

But we don't have hppa or IA-64 backends, so we're fine for now, I think. :)

lib/Frontend/InitPreprocessor.cpp

817

We should have a FIXME here to switch away from the non-standard "Q" when we can.

[hubert.reinterpretcast]

test/Sema/_Float128.cpp

1 ↗

(On Diff #124999)

GCC documents that it does not support _Float128 in C++ mode, and I think their decision makes sense:
On various targets, long double and __float128 are not the same type, but they decided to mangle them the same under -mlong-double-128 anyway. They also decided to make __float128 and _Float128 synonyms in the C mode on at least some of those targets.

The unfortunate state of affairs with the mangling may be okay for an extension type like __float128, but creating that situation with _Float128 does not seem wise. The test here is explicitly C++, so it seems this patch exposes _Float128 in C++ mode in a state that needs more discussion (and, in my opinion, one that should not ship).

[mibintc]

Thanks for all your reviews

[mibintc]

I changed the patch to enable _Float128 only as keyword in mode "nocxx" - this is the same mode being used by _Bool. I changed the test from .cpp to .c; I run check-all and saw only the usual suspects fail. What do you think?

[hubert.reinterpretcast]

lib/Frontend/InitPreprocessor.cpp

817

GCC does define these macros under with C++ mode when it would for the C mode, but with the F128 suffix (that it then interprets as a user-defined literal suffix). Use of these macros under C++ mode would behave differently between GCC and Clang until Q is replaced with F128.

In my experience, GCC on does not define these macros on platforms where __float128 is unsupported. This includes powerpc64le-linux-gnu without the -mfloat128 option.

test/Sema/_Float128.c

3

Duplicate run line

18

Add test that __FLT128_EPSILON__ is not defined?

[mibintc]

I responded to comments from @eli.friedman and @hubert.reinterpretcast : I added FIXME comment regarding the "Q" suffix on the float 128 literals (gcc uses F128 but clang doesn't support this). Hubert pointed out that the floating point macros should only be enabled when float128 is enabled, so I added a check before defining them. I fixed a couple problems that Hubert pointed out in the test case.

[mibintc]

added inline replies to Eli and Hubert

lib/Frontend/InitPreprocessor.cpp

817

I added a FIXME comment, and also added a check to see if float 128 is supported. Is this OK now?

[hubert.reinterpretcast]

Looks good to me.

[jgreenhalgh]

If this patch unconditionally defines _Float128, then I think it will conflict with the typedef for _Float128 for IEEE754 128-bit long double systems in glibc:

/* The type _Float128 exists only since GCC 7.0.  */
#  if !__GNUC_PREREQ (7, 0) || defined __cplusplus
typedef long double _Float128;
#  endif

https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=sysdeps/ieee754/ldbl-128/bits/floatn.h;hb=HEAD

This manifests on AArch64 as compile time failure as so:

$ cat x.c
#include <stdlib.h>
$ clang  x.c
In file included from x.c:1:
In file included from /usr/include/stdlib.h:55:
/usr/include/aarch64-linux-gnu/bits/floatn.h:67:21: error: cannot combine with previous 'double' declaration specifier
typedef long double _Float128;
                    ^
1 error generated.

I think that is an inadequate guard check in glibc, but perhaps there is something clang can do to help out?

Thanks,
James

[nsz]

if clang wants to provide _Float128 then the f128 constant suffix (specified by TS18661-3) and builtin_inff128, builtin_nanf128, builtin_nansf128, builtin_huge_valf128 (gcc builtins required by math.h) need to be supported too.

as this patch is committed clang is broken (cannot use glibc headers) on any target where long double is quad precision (aarch64, mips64, s390, sparc64, riscv64) even if glibc fixes the >=gcc-7 check to something that works for clang: either the _Float128 typedef conflicts with the definition by clang or the suffixes/builtins are missing.

[nsz]

also note that there is less than 3 weeks until glibc-2.27 is released, if the headers need a fix for clang then say so quickly

i opened https://sourceware.org/bugzilla/show_bug.cgi?id=22700 but it needs attention from some clang developers,
in particular there is no way to check if the compiler has _Float128 type defined but no working f128 suffix in the headers.

[efriedma]

as this patch is committed clang is broken (cannot use glibc headers)

This patch was supposed to *fix* compatibility with the glibc headers. If it doesn't, we should clearly revert it... but we need to figure out what we need to do to be compatible first.

From what I can see on this thread, we *must* define _Float128 on x86-64 Linux, and we *must not* define _Float128 for any other glibc target. Is that correct? Or does it depend on the glibc version?

(We have a bit of time before the 6.0 release, so we can adjust the behavior here to make it work. We probably don't want to try to add full _Float128 support on the branch, though.)

[nsz]

In D40673#973638, @efriedma wrote:

as this patch is committed clang is broken (cannot use glibc headers)

This patch was supposed to *fix* compatibility with the glibc headers. If it doesn't, we should clearly revert it... but we need to figure out what we need to do to be compatible first.

From what I can see on this thread, we *must* define _Float128 on x86-64 Linux, and we *must not* define _Float128 for any other glibc target. Is that correct? Or does it depend on the glibc version?

it is not clear to me from the original bug report what "fedora 27 workloads" break because of the lack of _Float128 type on x86. The glibc headers refer to _Float128, but normal include should not break unless the compiler claims to be >=gcc-7 or somebody explicitly requested the _Float128 support.

if clang defines _Float128 then the headers "work" on x86 as in simple math.h include is not broken, but some macro definitions will use the f128 const suffix (e.g. FLT128_MAX) which won't cause much breakage now but in the future users will want to know whether they can use these macros or not.

so either clang have to introduce all these features that are used by glibc together or provide ways for users (and glibc) to figure out what is supported and what isn't.

on non-x86 targets a glibc fix can solve the problem such that they "work" on the same level as x86, i.e. no immediate build breakage on most code, but some features are not supported. i think that's the right way forward, but it's not clear if anybody has time to do the header changes in glibc before release (it has to be tested on several targets).

if glibc is released as is today then those non-x86 targets don't want a _Float128 definition in clang-6 that breaks any math.h include on a glibc-2.27 system.

(We have a bit of time before the 6.0 release, so we can adjust the behavior here to make it work. We probably don't want to try to add full _Float128 support on the branch, though.)

include/clang/Basic/TokenKinds.def

 KEYWORD(__builtin_choose_expr       , KEYALL)
 KEYWORD(__builtin_offsetof          , KEYALL)
 // __builtin_types_compatible_p is a GNU C extension that we handle like a C++
 // type trait.
 TYPE_TRAIT_2(__builtin_types_compatible_p, TypeCompatible, KEYNOCXX)
 KEYWORD(__builtin_va_arg            , KEYALL)
 KEYWORD(__extension__               , KEYALL)
 KEYWORD(__float128                  , KEYALL)
+ALIAS("_Float128", __float128       , KEYNOCXX)
 KEYWORD(__imag                      , KEYALL)
 KEYWORD(__int128                    , KEYALL)
 KEYWORD(__label__                   , KEYALL)
 KEYWORD(__real                      , KEYALL)
 KEYWORD(__thread                    , KEYALL)
 KEYWORD(__FUNCTION__                , KEYALL)
 KEYWORD(__PRETTY_FUNCTION__         , KEYALL)
 KEYWORD(__auto_type                 , KEYALL)

lib/Frontend/InitPreprocessor.cpp

#undef TOSTR2
   DefineType("__UINTPTR_TYPE__", TI.getUIntPtrType(), Builder);
   DefineFmt("__UINTPTR", TI.getUIntPtrType(), TI, Builder);
   DefineTypeWidth("__UINTPTR_WIDTH__", TI.getUIntPtrType(), TI, Builder);
 
   DefineFloatMacros(Builder, "FLT16", &TI.getHalfFormat(), "F16");
   DefineFloatMacros(Builder, "FLT", &TI.getFloatFormat(), "F");
   DefineFloatMacros(Builder, "DBL", &TI.getDoubleFormat(), "");
   DefineFloatMacros(Builder, "LDBL", &TI.getLongDoubleFormat(), "L");
+  if (TI.hasFloat128Type())
+    // FIXME: Switch away from the non-standard "Q" when we can
+    DefineFloatMacros(Builder, "FLT128", &TI.getFloat128Format(), "Q");
+
 
   // Define a __POINTER_WIDTH__ macro for stdint.h.
   Builder.defineMacro("__POINTER_WIDTH__",
                       Twine((int)TI.getPointerWidth(0)));
 
   // Define __BIGGEST_ALIGNMENT__ to be compatible with gcc.
   Builder.defineMacro("__BIGGEST_ALIGNMENT__",
                       Twine(TI.getSuitableAlign() / TI.getCharWidth()) );

test/Preprocessor/cuda-types.cu

 // Check that types, widths, __GCC_ATOMIC* macros, etc. match on the host and
 // device sides of CUDA compilations.  Note that we filter out long double, as
 // this is intentionally different on host and device.
 //
 // FIXME: We really should make __GCC_HAVE_SYNC_COMPARE_AND_SWAP identical on
 // host and device, but architecturally this is difficult at the moment.
 
 // RUN: mkdir -p %t
 
 // RUN: %clang --cuda-host-only -nocudainc -target i386-unknown-linux-gnu -x cuda -E -dM -o - /dev/null \
 // RUN:   | grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF|WIDTH\)\|define __GCC_ATOMIC' \
-// RUN:   | grep -v '__LDBL\|_LONG_DOUBLE' > %t/i386-host-defines-filtered
+// RUN:   | grep -v '__FLT128\|__LDBL\|_LONG_DOUBLE' > %t/i386-host-defines-filtered
 // RUN: %clang --cuda-device-only -nocudainc -nocudalib -target i386-unknown-linux-gnu -x cuda -E -dM -o - /dev/null \
 // RUN:   | grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF|WIDTH\)\|define __GCC_ATOMIC' \
-// RUN:   | grep -v '__LDBL\|_LONG_DOUBLE' > %t/i386-device-defines-filtered
+// RUN:   | grep -v '__FLT128\|__LDBL\|_LONG_DOUBLE' > %t/i386-device-defines-filtered
 // RUN: diff %t/i386-host-defines-filtered %t/i386-device-defines-filtered
 
 // RUN: %clang --cuda-host-only -nocudainc -target x86_64-unknown-linux-gnu -x cuda -E -dM -o - /dev/null \
 // RUN:   | grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF|WIDTH\)\|define __GCC_ATOMIC' \
-// RUN:   | grep -v '__LDBL\|_LONG_DOUBLE' > %t/x86_64-host-defines-filtered
+// RUN:   | grep -v '__FLT128\|__LDBL\|_LONG_DOUBLE' > %t/x86_64-host-defines-filtered
 // RUN: %clang --cuda-device-only -nocudainc -nocudalib -target x86_64-unknown-linux-gnu -x cuda -E -dM -o - /dev/null \
 // RUN:   | grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF|WIDTH\)\|define __GCC_ATOMIC' \
-// RUN:   | grep -v '__LDBL\|_LONG_DOUBLE' > %t/x86_64-device-defines-filtered
+// RUN:   | grep -v '__FLT128\|__LDBL\|_LONG_DOUBLE' > %t/x86_64-device-defines-filtered
 // RUN: diff %t/x86_64-host-defines-filtered %t/x86_64-device-defines-filtered
 
 // RUN: %clang --cuda-host-only -nocudainc -target powerpc64-unknown-linux-gnu -x cuda -E -dM -o - /dev/null \
 // RUN:   | grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF|WIDTH\)\|define __GCC_ATOMIC' \
-// RUN:   | grep -v '__LDBL\|_LONG_DOUBLE' > %t/powerpc64-host-defines-filtered
+// RUN:   | grep -v '__FLT128\|__LDBL\|_LONG_DOUBLE' > %t/powerpc64-host-defines-filtered
 // RUN: %clang --cuda-device-only -nocudainc -nocudalib -target powerpc64-unknown-linux-gnu -x cuda -E -dM -o - /dev/null \
 // RUN:   | grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF|WIDTH\)\|define __GCC_ATOMIC' \
-// RUN:   | grep -v '__LDBL\|_LONG_DOUBLE' > %t/powerpc64-device-defines-filtered
+// RUN:   | grep -v '__FLT128\|__LDBL\|_LONG_DOUBLE' > %t/powerpc64-device-defines-filtered
 // RUN: diff %t/powerpc64-host-defines-filtered %t/powerpc64-device-defines-filtered
 
 // RUN: %clang --cuda-host-only -nocudainc -target i386-windows-msvc -x cuda -E -dM -o - /dev/null \
 // RUN:   | grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF|WIDTH\)\|define __GCC_ATOMIC' \
-// RUN:   | grep -v '__LDBL\|_LONG_DOUBLE' > %t/i386-msvc-host-defines-filtered
+// RUN:   | grep -v '__FLT128\|__LDBL\|_LONG_DOUBLE' > %t/i386-msvc-host-defines-filtered
 // RUN: %clang --cuda-device-only -nocudainc -nocudalib -target i386-windows-msvc -x cuda -E -dM -o - /dev/null \
 // RUN:   | grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF|WIDTH\)\|define __GCC_ATOMIC' \
-// RUN:   | grep -v '__LDBL\|_LONG_DOUBLE' > %t/i386-msvc-device-defines-filtered
+// RUN:   | grep -v '__FLT128\|__LDBL\|_LONG_DOUBLE' > %t/i386-msvc-device-defines-filtered
 // RUN: diff %t/i386-msvc-host-defines-filtered %t/i386-msvc-device-defines-filtered
 
 // RUN: %clang --cuda-host-only -nocudainc -target x86_64-windows-msvc -x cuda -E -dM -o - /dev/null \
 // RUN:   | grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF|WIDTH\)\|define __GCC_ATOMIC' \
-// RUN:   | grep -v '__LDBL\|_LONG_DOUBLE' > %t/x86_64-msvc-host-defines-filtered
+// RUN:   | grep -v '__FLT128\|__LDBL\|_LONG_DOUBLE' > %t/x86_64-msvc-host-defines-filtered
 // RUN: %clang --cuda-device-only -nocudainc -nocudalib -target x86_64-windows-msvc -x cuda -E -dM -o - /dev/null \
 // RUN:   | grep 'define __[^ ]*\(TYPE\|MAX\|SIZEOF|WIDTH\)\|define __GCC_ATOMIC' \
-// RUN:   | grep -v '__LDBL\|_LONG_DOUBLE' > %t/x86_64-msvc-device-defines-filtered
+// RUN:   | grep -v '__FLT128\|__LDBL\|_LONG_DOUBLE' > %t/x86_64-msvc-device-defines-filtered
 // RUN: diff %t/x86_64-msvc-host-defines-filtered %t/x86_64-msvc-device-defines-filtered

test/Sema/_Float128.c

+// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -triple powerpc64-linux -verify %s
+// RUN: %clang_cc1 -triple i686-windows-gnu -verify %s
+// RUN: %clang_cc1 -triple x86_64-windows-gnu -verify %s
+// RUN: %clang_cc1 -triple x86_64-windows-msvc -verify %s
+
+#if defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__)
+_Float128 f;
+_Float128 tiny = __FLT128_EPSILON__;
+int g(int x, _Float128 *y) {
+  return x + *y;
+}
+
+// expected-no-diagnostics
+#else
+_Float128 f;  // expected-error {{__float128 is not supported on this target}}
+float tiny = __FLT128_EPSILON__; // expected-error{{use of undeclared identifier}}
+int g(int x, _Float128 *y) {  // expected-error {{__float128 is not supported on this target}}
+  return x + *y;
+}
+
+#endif  // defined(__FLOAT128__) || defined(__SIZEOF_FLOAT128__)