Page MenuHomePhabricator

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

Authored by mibintc on Nov 30 2017, 1:29 PM.

Details

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?

Diff Detail

Event Timeline

mibintc created this revision.Nov 30 2017, 1:29 PM
mibintc added reviewers: erichkeane, cfe-commits.
mibintc removed subscribers: erichkeane, cfe-commits.
erichkeane added a subscriber: cfe-commits.
jlebar accepted this revision.Nov 30 2017, 1:35 PM
jlebar added a subscriber: jlebar.

LGTM for the CUDA test changes.

This revision is now accepted and ready to land.Nov 30 2017, 1:35 PM
rsmith added a subscriber: rsmith.Nov 30 2017, 1:57 PM

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.

_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 added inline comments.
test/Sema/_Float128.cpp
1

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 abandoned this revision.Dec 1 2017, 10:11 AM

Thanks for all your reviews

mibintc reclaimed this revision.Dec 1 2017, 1:18 PM
This revision is now accepted and ready to land.Dec 1 2017, 1:18 PM
mibintc updated this revision to Diff 125211.Dec 1 2017, 1:21 PM

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?

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
2 ↗(On Diff #125211)

Duplicate run line

17 ↗(On Diff #125211)

Add test that __FLT128_EPSILON__ is not defined?

mibintc updated this revision to Diff 125378.Dec 4 2017, 11:28 AM
mibintc added a subscriber: eli.friedman.

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 marked 3 inline comments as done.Dec 4 2017, 11:30 AM

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?

Looks good to me.

This revision was automatically updated to reflect the committed changes.

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 added a subscriber: nsz.Jan 11 2018, 3:13 AM

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 added a comment.Jan 11 2018, 3:24 AM

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.

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 added a comment.Jan 12 2018, 12:18 PM

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.)