This is an archive of the discontinued LLVM Phabricator instance.

Enable __float128 on X86 and SystemZ
ClosedPublic

Authored by nemanjai on Apr 14 2016, 10:40 AM.

Details

Summary

It would appear that gcc supports the __float128 type on X86_64. Systems with this architecture that I've seen have the GNU C++ library configured with _GLIBCXX_USE_FLOAT128 defined. This in turn is used in at least one header (type_traits) and there is one template specialization with this type in the header. As a result, Clang no longer compiles this header with -std=gnu++11.

This patch enables this type on X86_64 targets.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai updated this revision to Diff 53752.Apr 14 2016, 10:40 AM
nemanjai retitled this revision from to Enable __float128 on X86_64.
nemanjai updated this object.
nemanjai added reviewers: echristo, bruno, nadav, hfinkel.
nemanjai set the repository for this revision to rL LLVM.
nemanjai added a subscriber: llvm-commits.

Why not enable __float128 on Linux/i386, which is defined in
the current i386 psABI?

Why not enable __float128 on Linux/i386, which is defined in
the current i386 psABI?

I am certainly more than happy to do so. The issue is that I really don't know what set of targets have this enabled (i.e. I don't know how this config macro is determined). Can you please post a link to this i386 psABI so I can get agreement between all the reviewers?

bruno edited edge metadata.Apr 14 2016, 11:09 AM

I agree with H.J Lu.

Looks like test/CodeGenCXX/float128-declarations.cpp deserves some x86 RUN lines too.

Some comments inline.

lib/Basic/Targets.cpp
4177

clang-format this one, looks like it could be a onliner like the other aboves, also place it below hasInt128Type().

test/Sema/128bitfloat.cpp
4

Better to have a x86-128bitfloat.cpp file instead, targetting i386/x86_64 with two different RUN lines. This ensures this feature gets tested when host isn't x86.

nemanjai updated this revision to Diff 53848.Apr 15 2016, 12:32 AM
nemanjai retitled this revision from Enable __float128 on X86_64 to Enable __float128 on X86 and SystemZ.
nemanjai added a reviewer: jnspaulsson.

Updated the patch to address the comments:

  1. Enabled __float128 for Linux targets (x86, x86_64 and systemz). Both x86 and x86_64 seem to have the support mentioned in the psABI and SystemZ has IEEE Quad precision support which this type is supposed to target.
  2. Updated the test case for the targets that now have this type enabled. Please note that I have not updated the test case that expects diagnostics but rather the test case that tests this type in various declaration contexts.
nemanjai added inline comments.Apr 15 2016, 12:36 AM
lib/Basic/Targets.cpp
437

I am not sure this is the right way to do this, but it seems to encapsulate the requested semantics (i.e. Linux/i386...).

I am sorry to be pinging the reviewers already, but I'm wondering if we can get this patch sorted out and committed so I don't have to revert the patch that enables __float128 as it is a large patch and otherwise fine. I've already received a request to revert the initial patch and I suppose I'll have to do that if I can't get this one approved today.

jonpa added a subscriber: jonpa.Apr 21 2016, 1:57 AM

I tried running this on SystemZ, by reverting your revert commit.

I added

bool hasFloat128Type() const override {

return true;

}

in the SystemZTargetInfo definition.

test/Sema/128bitfloat.cpp fails, because of an expected_error: "__float128 is not supported on this target"

Otherwise, all tests seem to pass, including the test-suite.

nemanjai added inline comments.Apr 21 2016, 7:41 AM
test/Sema/128bitfloat.cpp
4

I think it'll be better if I change this to

#ifdef __FLOAT228__

A gentle reminder to all reviewers that this patch is holding up the addition of support for the __float128 type that I am hoping to commit. Please review.

hjl.tools edited edge metadata.Apr 26 2016, 5:33 AM

Should it be handled similar to int128? Only targets which support float128
override hasFloat128Type without adding HasFloat128.

Should it be handled similar to int128? Only targets which support float128
override hasFloat128Type without adding HasFloat128.

I implemented it this way so that it isn't necessarily tied to an architecture, but rather the specific architectures on Linux. My hope was to match your suggestion to enable it for Linux/i386. However, I am certainly open to suggestions as I really don't know exactly what the relationship is between __float128 and architecture/OS.

I really need to re-commit the changes for __float128 in the front end and this review is holding it up. Can I kindly ask the reviewers to have another look at this patch and give me some further direction?

echristo accepted this revision.May 3 2016, 2:24 PM
echristo edited edge metadata.

Couple of inline comments, but this works for me provided answers are OK to the questions inline.

-eric

lib/Basic/Targets.cpp
437

Seems ok. What happened to the ppc64 lines? (I don't particularly like this solution, but it's fine for now)

test/Sema/128bitfloat.cpp
4

Modulo typo, but I think that works yeah.

This revision is now accepted and ready to land.May 3 2016, 2:24 PM

I have made the update to the test case (without the typo). Also, added the FLOAT128 define where the target feature is set.
Committed revision 268898.

nemanjai closed this revision.May 9 2016, 2:03 AM

GCC has never supported the __float128 type on SystemZ, because "long double" is already IEEE-128 on the platform. GCC only supports a separate __float128 type on platforms where "long double" is some other type (like x86 or ppc64).

For compatibility with GCC I've removed support for SystemZ again as rev. 348247.