Skip to content
This repository has been archived by the owner on May 7, 2024. It is now read-only.

Add -mlong-double-64 option #122

Open
wants to merge 1 commit into
base: riscv-gcc-7.2.0
Choose a base branch
from
Open

Conversation

aswaterman
Copy link
Contributor

@aswaterman aswaterman commented Feb 15, 2018

Putting this here for discussion purposes.

Embedded targets don't generally want the glut of emulation code that float128 requires. This switch enables an alternate ABI that aliases long double to double, which is permitted by the C standard. RVIFD targets will then need no emulation code, and RVI/RVF targets will need less.

The default should remain -mlong-double-128, since that's what the standard ABIs expect.

@jim-wilson
Copy link
Collaborator

A number of targets have 64-bit long doubles, so this is a reasonable change to discuss. The option needs a doc/invoke.texi patch to document it.

However, this is an ABI change. Currently, our ABIs are fully expressed by the -mabi option. This would no longer be the case. That may or may not be a problem. Newlib/glibc aren't going to work if they aren't compiled with the option, suggesting that we may need to add more multilibs to make it work for embedded targets, and maybe disallow it for linux targets where it will be too much trouble to make it work, because we really don't want a linux ABI change. We need to consider what happens if different size long double code is linked together, we might need another elf header flag to indicate long double size so that the linker can give an error for this. Or maybe if the ELF attribute stuff comes soon enough we can use an ELF attribute to detect the mismatch. We probably need new predefined macros so code can check for the long double size. There may also be other issues that don't immediately come to mind.

That of course is if you want the option to be user friendly. We could just document the option as violating the ABI, and unlikely to work unless you compile all code with it including your libraries.

@aswaterman
Copy link
Contributor Author

At a minimum, an additional ELF e_flags bit or ELF attribute seems prudent, just to catch obviously faulty links. Beyond that, I'm not sure how much more user-friendly we need to be. My guess is that families of embedded targets are going to go all one way or all the other way, so maybe even multilib support can be agnostic to this setting.

@palmer-dabbelt
Copy link
Contributor

I think we just add an ELF flag and document it as changing the ABI. IIRC that's what we do for the stack alignment stuff, which is another subtle ABI issue. I don't think it's worth shipping two copies of the libraries.

It'd be really cool if there was some way to only make the libraries incompatible if you actually use long double, as I bet a lot of code won't.

@sorear
Copy link

sorear commented Feb 17, 2018

Does this change affect max_align_t and/or stack and alloca alignment?

@aswaterman
Copy link
Contributor Author

Thanks for raising that point. I propose this option not reduce alignment.

@jim-wilson
Copy link
Collaborator

A run-time option won't work unless we multilib libraries, and we already have too many multilibs. A configure time option might be better.

With 64-bit long doubles, libgcc fails to build. The libgcc/config/riscv files and the libgcc/config.host support for riscv assumes that TFmode always exists as a 128-bit type, so we would need changes here to make multilibbing or a configure time option work. The workaound to make a hard-coded 64-bit long double work is in libgcc/config.host replace riscv/t-softfp${host_address} t-softfp with t-softfp-sfdf.

@aswaterman
Copy link
Contributor Author

Makes sense.

@solomatnikov
Copy link

This is required to compile:

diff --git a/libgcc/config.host b/libgcc/config.host
index bedcf10..1f00022 100644
--- a/libgcc/config.host
+++ b/libgcc/config.host
@@ -1097,12 +1097,12 @@ powerpcle-*-eabi*)
        extra_parts="$extra_parts crtbegin.o crtend.o crtbeginS.o crtendS.o crtbeginT.o ecrti.o ecrtn.o ncrti.o ncrtn.o"
        ;;
 riscv*-*-linux*)
-       tmake_file="${tmake_file} riscv/t-softfp${host_address} t-softfp riscv/t-elf riscv/t-elf${host_address}"
+       tmake_file="${tmake_file} t-softfp-sfdf t-softfp riscv/t-elf riscv/t-elf${host_address}"
        extra_parts="$extra_parts crtbegin.o crtend.o crti.o crtn.o crtendS.o crtbeginT.o"
        md_unwind_header=riscv/linux-unwind.h
        ;;
 riscv*-*-*)
-       tmake_file="${tmake_file} riscv/t-softfp${host_address} t-softfp riscv/t-elf riscv/t-elf${host_address}"
+       tmake_file="${tmake_file} t-softfp-sfdf t-softfp riscv/t-elf riscv/t-elf${host_address}"
        extra_parts="$extra_parts crtbegin.o crtend.o crti.o crtn.o"
        ;;
 rs6000-ibm-aix4.[3456789]* | powerpc-ibm-aix4.[3456789]*)

@solomatnikov
Copy link

@palmer-dabbelt @jim-wilson when do you think this will be merged/upstreamed?

@jim-wilson
Copy link
Collaborator

It is an ABI breaking change. It shouldn't be upstreamed as is. I have a larger patch that tries to do it right, including binutils changes for the ABI change, but I'm not happy with that patch either. I think this needs discussion at a higher level about defining new ABIs.

@aswaterman
Copy link
Contributor Author

Is this rendered irrelevant by ongoing EABI work?

@jim-wilson
Copy link
Collaborator

The plan is to include this change in the EABI, since that requires an ABI break anyways. That gives us a single ABI break instead of two.

@kito-cheng
Copy link
Collaborator

Hi @jim-wilson, where can I find more detail about EABI? last time I saw that is in a discussion thread from fast-interrupter TG?

@jim-wilson
Copy link
Collaborator

There hasn't been any progress on the EABI since last summer. We've all been busy with other stuff. Krste is busy with vector stuff at the moment, and probably won't get back to the EABI for a while.

@kito-cheng
Copy link
Collaborator

OK, thanks :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants