Page MenuHomePhabricator

[X86][PPC] Support -mlong-double-64
ClosedPublic

Authored by MaskRay on Jul 2 2019, 4:32 AM.

Details

Summary

-mlong-double-64 is supported on some ports of gcc (i386, x86_64, and ppc{32,64}).
On many other targets, there will be an error:

error: unrecognized command line option '-mlong-double-64'

This patch makes the driver option -mlong-double-64 available for x86
and ppc. The CC1 option -mlong-double-64 is available on all targets for
users to test on unsupported targets.

LongDoubleSize is added as a VALUE_LANGOPT so that the option can be
shared with -mlong-double-128 when we support it in clang.

Also, make powerpc*-linux-musl default to use 64-bit long double. It is
currently the only supported ABI on musl and is also how people
configure powerpc*-linux-musl-gcc.

Diff Detail

Repository
rL LLVM

Event Timeline

MaskRay created this revision.Jul 2 2019, 4:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2019, 4:32 AM
MaskRay updated this revision to Diff 207532.Jul 2 2019, 6:04 AM

Improve a test

rnk added a comment.Jul 2 2019, 1:26 PM

Sounds good to me. I had a minor suggestion, though.

lib/Basic/Targets/PPC.cpp
468 ↗(On Diff #207532)

I think it would be preferable to implement this in the base TargetInfo::adjust implementation, just to reduce code duplication. The driver already checks that the option makes sense for some particular target. I think it's actually kind of nice that the user can bypass the driver with -Xclang and try using it for some unsupported target to see what happens. It will probably work anyway.

MaskRay updated this revision to Diff 207695.Jul 2 2019, 6:54 PM
MaskRay edited the summary of this revision. (Show Details)

Implement this in TargetInfo::adjust as rnk suggested

MaskRay marked an inline comment as done.Jul 2 2019, 6:59 PM

Can you please add a test ensuring that -malign-double and -mlong-double-64 interact properly? I think that, in the current patch, they do (as -malign-double is processed first), but I'd prefer that we cover that case explicitly.

MaskRay updated this revision to Diff 207708.Jul 2 2019, 10:41 PM

Check -malign-double

MaskRay updated this revision to Diff 207795.Jul 3 2019, 7:41 AM

Make powerpc* musl default to 64-bit long double

In this review (https://reviews.llvm.org/D6260) @rsmith mentions that this should also have an effect on name mangling.

What will this do if the user calls a library function that expects a long double? What does gcc do in that case?

rnk added a comment.Jul 3 2019, 11:16 AM

In this review (https://reviews.llvm.org/D6260) @rsmith mentions that this should also have an effect on name mangling.

I'm not sure that's consistent with GCC, at least not anymore:
https://gcc.godbolt.org/z/eUviCd
Looks like you can still have an overload set with double and long double, even if they both use the same representation. This is a backend -m flag, after all, so that seems reasonable to me.

What will this do if the user calls a library function that expects a long double? What does gcc do in that case?

Looks like it passes according to the usual 64-bit IEEE double representation.

In D64067#1568888, @rnk wrote:

In this review (https://reviews.llvm.org/D6260) @rsmith mentions that this should also have an effect on name mangling.

I'm not sure that's consistent with GCC, at least not anymore:
https://gcc.godbolt.org/z/eUviCd
Looks like you can still have an overload set with double and long double, even if they both use the same representation.

It has to work that way, because they're different, standard language-level types.

One thing to realize about these flags is that they're ABI-altering flags. If the user provides the flag to alter the platform defaults, this only works if the user also ensures that matches the ABI of the relevant system libraries that the compiler is using (e.g., because the user is explicitly linking with a suitable libc).

This is a backend -m flag, after all, so that seems reasonable to me.

What will this do if the user calls a library function that expects a long double? What does gcc do in that case?

Looks like it passes according to the usual 64-bit IEEE double representation.

One thing to realize about these flags is that they're ABI-altering flags. If the user provides the flag to alter the platform defaults, this only works if the user also ensures that matches the ABI of the relevant system libraries that the compiler is using (e.g., because the user is explicitly linking with a suitable libc).

Right. That's what I was trying to figure out. Are we relying on the users of this option not to shoot themselves in the foot? It sounds like yes. I believe that's the way we handled it in icc also, but gcc and icc do a lot of sketchy things that we wouldn't want to do in clang. In this case I don't object to the sketchiness, just so everyone realizes that's what we're doing.

MaskRay updated this revision to Diff 207965.Jul 3 2019, 8:37 PM
MaskRay edited the summary of this revision. (Show Details)

Add tests to check mangling of long double

-mlong-double-64 has no effect on gcc x86.
On gcc ppc, -mlong-double-64 changes the mangled type from 'g' (ibmlongdouble) to 'e'

MaskRay added a comment.EditedJul 3 2019, 9:18 PM
% g++ a.cc -S -o - | grep '^_Z3foo'
_Z3fooe:
% g++ a.cc -S -o - -mlong-double-64 | grep '^_Z3foo'
_Z3fooe:
% g++ a.cc -S -o - -mlong-double-128 | grep '^_Z3foo'
_Z3foog:

% powerpc64le-linux-gnu-g++ a.cc -S -o - -mlong-double-64 | grep '^_Z3foo'
_Z3fooe:
% powerpc64le-linux-gnu-g++ a.cc -S -o - -mlong-double-128 -mabi=ibmlongdouble | grep '^_Z3foo'
_Z3foog:
% powerpc64le-linux-gnu-g++ a.cc -S -o - -mlong-double-128 -mabi=ieeelongdouble | grep '^_Z3foo'
_Z3fooU10__float128:

With this patch, the mangling scheme of -mlong-double-64 is consistent with gcc on x86 and ppc. (-mlong-double-128 is not supported now)

% clang a.cc -S -o - -mlong-double-64 | grep '^_Z3foo'        
_Z3fooe:                                # @_Z3fooe
% clang a.cc -S -o - | grep '^_Z3foo'        
_Z3fooe:                                # @_Z3fooe

% clang -target ppc64le a.cc -S -o - -mlong-double-64 | grep '^_Z3foo'
_Z3fooe:                                # @_Z3fooe
% clang -target ppc64le a.cc -S -o - | grep '^_Z3foo'  # corresponds to gcc -mlong-double-128 -mabi=ibmlongdouble
_Z3foog:                                # @_Z3foog
MaskRay updated this revision to Diff 208255.Jul 5 2019, 11:33 PM

Add more tests: elf32/elf64/darwin...

rnk accepted this revision.Jul 8 2019, 4:58 PM

lgtm

This revision is now accepted and ready to land.Jul 8 2019, 4:58 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2019, 5:27 PM
troyj added a subscriber: troyj.Jul 18 2019, 12:43 PM

Hi, we just inherited this commit at Cray when we did our latest upstream merge and there are a few problems with it that I'd like to point out. Sorry that I was not part of the initial discussion here, but I didn't know that this work was being done and I had already done it for x86 in our downstream compiler a while ago.

  1. This patch adds the -m options to f_Group, which is weird. They should be in m_Group since they are target-specific. I actually implemented them as part of a subgroup of m_Group so that I can take the last option from that group that appears on the command line.
  2. This patch lacks the GNU -mlong-double-80 option for x86.
  3. Instead of tracking the size in clang/Basic/LangOptions.def, I track it in clang/Basic/TargetOptions.h. This is a target-specific thing afterall.

We're trying to resolve merge conflicts with this change and might be able to submit a patch to help reduce our differences. Would that be of interest?

Hi, we just inherited this commit at Cray when we did our latest upstream merge and there are a few problems with it that I'd like to point out. Sorry that I was not part of the initial discussion here, but I didn't know that this work was being done and I had already done it for x86 in our downstream compiler a while ago.

  1. This patch adds the -m options to f_Group, which is weird. They should be in m_Group since they are target-specific. I actually implemented them as part of a subgroup of m_Group so that I can take the last option from that group that appears on the command line.
  2. This patch lacks the GNU -mlong-double-80 option for x86.
  3. Instead of tracking the size in clang/Basic/LangOptions.def, I track it in clang/Basic/TargetOptions.h. This is a target-specific thing afterall.

    We're trying to resolve merge conflicts with this change and might be able to submit a patch to help reduce our differences. Would that be of interest?

Yes, that would be helpful.

rnk added a comment.Jul 18 2019, 1:17 PM

Hi, we just inherited this commit at Cray when we did our latest upstream merge and there are a few problems with it that I'd like to point out. Sorry that I was not part of the initial discussion here, but I didn't know that this work was being done and I had already done it for x86 in our downstream compiler a while ago.

  1. This patch adds the -m options to f_Group, which is weird. They should be in m_Group since they are target-specific. I actually implemented them as part of a subgroup of m_Group so that I can take the last option from that group that appears on the command line.

Oops, seems like a bug.

  1. This patch lacks the GNU -mlong-double-80 option for x86.
  2. Instead of tracking the size in clang/Basic/LangOptions.def, I track it in clang/Basic/TargetOptions.h. This is a target-specific thing afterall.

Sema needs to know about this option for sizeof(long double). I believe that's the guiding line between the various option kinds. However, it means almost everything ends up being a language option, for better or worse.

We're trying to resolve merge conflicts with this change and might be able to submit a patch to help reduce our differences. Would that be of interest?

Sounds good.