This is an archive of the discontinued LLVM Phabricator instance.

Add -mlong-double-64 flag
Needs ReviewPublic

Authored by tzik on Nov 13 2014, 9:35 PM.

Details

Summary

Fixes PR21543.
This patch adds -mlong-double-64 flag to cfe to override the size of long double.
GCC supports this flag and uses for Bionic.

Diff Detail

Event Timeline

tzik updated this revision to Diff 16195.Nov 13 2014, 9:35 PM
tzik retitled this revision from to Add -mlong-double-64 flag.
tzik updated this object.
tzik edited the test plan for this revision. (Show Details)
tzik updated this object.Nov 13 2014, 9:52 PM
tzik added a reviewer: kcc.
tzik added a subscriber: Unknown Object (MLST).
tzik added a comment.Nov 13 2014, 10:03 PM

Hi, Kostya.
Could you take a look to this?

We are trying to build Bionic with clang, though it get stuck in a issue due to the size mismatch of long double.
Can we add the flag to deal with it?

kcc added a reviewer: rsmith.Nov 14 2014, 1:08 PM

The patch looks ok, but I am not the person to approve patches in this part of clang.
CC-ed rsmith.

rnk added a subscriber: rnk.Nov 14 2014, 1:15 PM

This needs an IRGen test. We should produce the LLVM 'double' type instead of the x86_fp80 type.

tzik updated this revision to Diff 16282.Nov 17 2014, 3:13 AM
tzik updated this object.
tzik edited edge metadata.

Add CodeGen test.
Update LongDoubleFormat.

tzik updated this revision to Diff 16283.Nov 17 2014, 3:25 AM

Drop unrelated whitespace changes

tzik added a comment.Nov 17 2014, 3:28 AM
In D6260#8, @rnk wrote:

This needs an IRGen test. We should produce the LLVM 'double' type instead of the x86_fp80 type.

OK, I added a test to the CL.

I think another things to consider is LongDoubleAlign.
LongDoubleAlign is 32 on x86_32 and 64 on x86_64, should I adjust it in TargetInfo::adjust too for each supported arch?

tzik added a comment.Dec 11 2014, 10:33 PM

rnk@, rsmith@: Could you take another look?
Is this kind of CL unacceptable by design? Or, it just needs refinement?

chandlerc edited reviewers, added: echristo, timshen, dlj; removed: kcc.Jun 21 2018, 12:52 AM
chandlerc edited subscribers, added: brooksmoses, chandlerc; removed: rnk.

Ok folks, I know this is a blast from the past, but can we actually review this? I'm willing to commandeer it, rebase it, and get it landed if you all can review this and confirm that it is the right direction.

hfinkel added inline comments.
lib/Basic/TargetInfo.cpp
277

This seems wrong for targets in general. Maybe this should be, instead of 32 always, 64 on 64-bit targets?

What about -mlong-double-128? It seems sensible to me to add support for that at the same time.

It looks like this also needs to affect name mangling. As far as I can tell from some quick testing, on ppc64-linux-gnu GCC uses e for 64-bit long double and g for 128-bit long double. On x86_64, -mlong-double-64 and -mlong-double-128 do not appear to affect the mangling scheme. Someone needs to do some research here to find out what the right mangling scheme to use is for each platform where we want to allow these flags.

lib/Basic/TargetInfo.cpp
277

I'd expect that this should match the alignment of double.

299

What should happen if you try to combine -mlong-double-64 with -x cl? I'd expect the -mlong-double-64 to win (as it's clearly a more specific override).