| Message ID | 20210301171852.GB9185@ibm-toto.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
| Series | Honor --disable-decimal-float in PowerPC libgcc _Float128 | expand |
On Mon, Mar 01, 2021 at 12:18:52PM -0500, Michael Meissner wrote: > The _sprintfkf.c file was including stdio.h to get the definition of sprintf. (declaration of) > This patch modifies this so that stdio.h is not included in order to support > freestanding cross compilers that might not provide stdio.h. So the code cannot work at all there? Will not link? > + We use __builtin_sprintf so that we don't have to include stdio.h to define > + sprintf. Stdio.h might not be present for freestanding cross compilers that > + do not need to include a library. */ "declare sprintf", and the function is called stdio.g (all lowercase). It is often written <stdio.h>, which makes it easier to handle in sentences. > @@ -54,5 +57,5 @@ int __sprintfkf (char *restrict string, > if (__sprintfieee128) > return __sprintfieee128 (string, format, number); > > - return sprintf (string, format, (long double) number); > + return __builtin_sprintf (string, format, (long double) number); sprintf as well as __builtin_sprintf do not exist exactly when <stdio.h> does not (or are not guaranteed to exist, anyway). Segher
On Mon, Mar 01, 2021 at 05:15:44PM -0600, Segher Boessenkool wrote: > On Mon, Mar 01, 2021 at 12:18:52PM -0500, Michael Meissner wrote: > > The _sprintfkf.c file was including stdio.h to get the definition of sprintf. > > (declaration of) > > > This patch modifies this so that stdio.h is not included in order to support > > freestanding cross compilers that might not provide stdio.h. > > So the code cannot work at all there? Will not link? > > > + We use __builtin_sprintf so that we don't have to include stdio.h to define > > + sprintf. Stdio.h might not be present for freestanding cross compilers that > > + do not need to include a library. */ > > "declare sprintf", and the function is called stdio.g (all lowercase). > It is often written <stdio.h>, which makes it easier to handle in > sentences. > > > @@ -54,5 +57,5 @@ int __sprintfkf (char *restrict string, > > if (__sprintfieee128) > > return __sprintfieee128 (string, format, number); > > > > - return sprintf (string, format, (long double) number); > > + return __builtin_sprintf (string, format, (long double) number); > > sprintf as well as __builtin_sprintf do not exist exactly when <stdio.h> > does not (or are not guaranteed to exist, anyway). There are 2 issues: The first issue is whether you get an error when you are building a cross compiler and the target you are using does not provide a stdio.h. This is the error that this patch fixes. I tend to regard not being able to build the toolchain are higher in priority than whether it works at runtime. The second is whether you get an error at link time because there is not sprintf or strtold functions. For normal archive libraries, this would only be an issue if you actually do the conversion. I have my doubts whether there is any extant code that wants to convert _Float128 to one of the Decimal types or vice versa. Note the second issue would affect x86_64 cross compilers as well, since they use those two functions to do the _Float128/Decimal versions. I am open to suggestions of how to move forward. I think we have to have a way to build cross compilers without decimal support (i.e. my third patch). Secondarily, I think we should allow the compiler to be built if there is no stdio.h, but the user did not disable decimal floating point. I don't think rewriting the Decimal conversions not to use GLIBC is really practical. I certainly do not have the FP math skills to do this without losing precision, etc.
On Tue, Mar 02, 2021 at 04:25:33PM -0500, Michael Meissner wrote: > On Mon, Mar 01, 2021 at 05:15:44PM -0600, Segher Boessenkool wrote: > > On Mon, Mar 01, 2021 at 12:18:52PM -0500, Michael Meissner wrote: > > > The _sprintfkf.c file was including stdio.h to get the definition of sprintf. > > > > (declaration of) > > > > > This patch modifies this so that stdio.h is not included in order to support > > > freestanding cross compilers that might not provide stdio.h. > > > > So the code cannot work at all there? Will not link? > > > > > + We use __builtin_sprintf so that we don't have to include stdio.h to define > > > + sprintf. Stdio.h might not be present for freestanding cross compilers that > > > + do not need to include a library. */ > > > > "declare sprintf", and the function is called stdio.g (all lowercase). > > It is often written <stdio.h>, which makes it easier to handle in > > sentences. > > > > > @@ -54,5 +57,5 @@ int __sprintfkf (char *restrict string, > > > if (__sprintfieee128) > > > return __sprintfieee128 (string, format, number); > > > > > > - return sprintf (string, format, (long double) number); > > > + return __builtin_sprintf (string, format, (long double) number); > > > > sprintf as well as __builtin_sprintf do not exist exactly when <stdio.h> > > does not (or are not guaranteed to exist, anyway). > > There are 2 issues: > > The first issue is whether you get an error when you are building a cross > compiler and the target you are using does not provide a stdio.h. This is the > error that this patch fixes. I tend to regard not being able to build the > toolchain are higher in priority than whether it works at runtime. And I see both as basic requirements. A patch that adds X should make X work. If you want to make decimal and/or QP float work only on 64-bit LE Linux you should say so. And in that case, that is certainly not acceptable if it doesn't "sorry" at configure time already. > The second is whether you get an error at link time because there is not > sprintf or strtold functions. For normal archive libraries, this would only be > an issue if you actually do the conversion. I have my doubts whether there is > any extant code that wants to convert _Float128 to one of the Decimal types or > vice versa. So what are these patches for at all, again? Anyway, if some conversion doesn't work (or cannot work correctly at all, which is the same thing), you should simply disable the conversion routines themselves (and then cascade that through the possible callers:just make them say "sorry, unimplemented"). > Note the second issue would affect x86_64 cross compilers as well, since they > use those two functions to do the _Float128/Decimal versions. Yes, it cannot work correctly at all there, either. > I am open to suggestions of how to move forward. I think we have to have a way > to build cross compilers without decimal support (i.e. my third patch). > Secondarily, I think we should allow the compiler to be built if there is no > stdio.h, but the user did not disable decimal floating point. Yes. So just do not use it then. Disable the feature that would use it. > I don't think rewriting the Decimal conversions not to use GLIBC is really > practical. It is necessary if you want to support it on any other systems than the one you use. Segher
On Tue, Mar 02, 2021 at 03:53:06PM -0600, Segher Boessenkool wrote: > If you want to make decimal and/or QP float work only on 64-bit LE Linux > you should say so. And in that case, that is certainly not acceptable > if it doesn't "sorry" at configure time already. Well in general the only supported configuration for _Float128 is 64-bit LE Linux, but this is more due to the infrastructure not supporting it. If you want to support _Float128 on big endian, you need a GLIBC that provides the necessary support. As far as I know, GLIBC does not build the _Float128 support on big endian. You also need to set the minimum CPU to power7, because _Float128 is passed in Altivec registers. I think some of the configure tests use the default cpu used in building GCC (i.e. power4 if you don't use --with-cpu). As we have discussed many times, on 32-bit BE, you cannot use hardware _Float128 support on power9/power10 because there is no TImode in 32-bit. Various machine independent parts of GCC require an integer type to be the same size as basic types. If somebody made TImode work, we can remove the restriction and allow _Float128 to work on 32-bit. With the current compiler on BE, if you use -mcpu=power7 or newer, it will enable the _Float128/__float128 keywords, and generate the code. But until there is an expectation of library support, it won't work for the general user. > > The second is whether you get an error at link time because there is not > > sprintf or strtold functions. For normal archive libraries, this would only be > > an issue if you actually do the conversion. I have my doubts whether there is > > any extant code that wants to convert _Float128 to one of the Decimal types or > > vice versa. > > So what are these patches for at all, again? The whole reason for the patches is to provide the support when we flip the default long double type from the IBM 128-bit type to the IEEE 128-bit type that conversions between long double and the Decimal types will succeed. I suspect in real life it won't be an issue, since Decimal is not used that much. But it is more likely people will want to convert between binary long double and Decimal128. I missed the fact that it had hidden dependencies for cross compilers. So I am trying to fix this. > Anyway, if some conversion doesn't work (or cannot work correctly at > all, which is the same thing), you should simply disable the conversion > routines themselves (and then cascade that through the possible callers:just make them say "sorry, unimplemented"). > > > Note the second issue would affect x86_64 cross compilers as well, since they > > use those two functions to do the _Float128/Decimal versions. > > Yes, it cannot work correctly at all there, either. If you remember, the original versions of the patch would only work if you configured the compiler to use GLIBC 2.32 or newer (such as using the --with-advance-toolchain at14.0 option). You did not like this because as you pointed out, you might use a different GLIBC when linking. So I wrote the current patch that uses weak references to see if we did link with GLIBC 2.32. If the library is present, we use the functions in that library. If not, we have to give an approximate answer. I used the existing IBM 128-bit support to do the conversion. Given GLIBC 2.32 is the minimum version of GLIBC that supports IEEE 128-bit long doubles, if you link against an earlier library, you will not get the conversions from long double. You will only get the conversion if you explicitly use _Float128. > > I am open to suggestions of how to move forward. I think we have to have a way > > to build cross compilers without decimal support (i.e. my third patch). > > Secondarily, I think we should allow the compiler to be built if there is no > > stdio.h, but the user did not disable decimal floating point. > > Yes. So just do not use it then. Disable the feature that would use it. > > > I don't think rewriting the Decimal conversions not to use GLIBC is really > > practical. > > It is necessary if you want to support it on any other systems than the > one you use. Sure, but it is a massive amount of work. And I don't have the necessary skills to insure that the conversion process will not suffer from errors.
On Wed, 3 Mar 2021, Michael Meissner via Gcc-patches wrote: > As we have discussed many times, on 32-bit BE, you cannot use hardware > _Float128 support on power9/power10 because there is no TImode in 32-bit. > Various machine independent parts of GCC require an integer type to be the same > size as basic types. If somebody made TImode work, we can remove the > restriction and allow _Float128 to work on 32-bit. I'm not sure exactly what the machine-independent requirement is, but _Float128 is supported for 32-bit x86, riscv, sparc and s390 at least. There's no support for _Float128 in the 32-bit powerpc ABI, but I don't think there is anything architecture-independent preventing such support on 32-bit platforms where TImode is unsupported (i.e. not supported by the scalar_mode_supported_p hook).
On Wed, Mar 03, 2021 at 11:33:52PM +0000, Joseph Myers wrote: > On Wed, 3 Mar 2021, Michael Meissner via Gcc-patches wrote: > > > As we have discussed many times, on 32-bit BE, you cannot use hardware > > _Float128 support on power9/power10 because there is no TImode in 32-bit. > > Various machine independent parts of GCC require an integer type to be the same > > size as basic types. If somebody made TImode work, we can remove the > > restriction and allow _Float128 to work on 32-bit. > > I'm not sure exactly what the machine-independent requirement is, but > _Float128 is supported for 32-bit x86, riscv, sparc and s390 at least. > > There's no support for _Float128 in the 32-bit powerpc ABI, but I don't > think there is anything architecture-independent preventing such support > on 32-bit platforms where TImode is unsupported (i.e. not supported by the > scalar_mode_supported_p hook). The problem is several parts of GCC insists that there be an integer MODE that is the same size as the scalar MODE. I recall it happens in moves (such as during calls), but it could be other places as well. In theory, it would be nice to get this fixed, but the practicality is you can go down a lot of places to find the next bug and fix that. Maybe there is somebody who has enough time and patience to fix all of the places that demand that there be an integer type large enough to hold scalar types like _Float128 and generate alternate code. Or perhaps implement enough of TImode so that it doesn't cause issues. But in order to do the work and make sure it does not cause issues for 32-bit and hardware _Float128, you need to build on a big endian power9 system. In the compile farm there is gcc135, but it is run little endian. BTW, during the initial development of MMA, I ran into the same issue for the vector pair and vector quad types. Each of the approaches that we took had some serious issues. What I did in the initial stages of development is add dummy moves that would never succeed for 256 and 512-bit integers, and then use partial integers to represent that these weren't quite real integers. However, even then we discovered various parts of the compiler would strip off the partial integer type and use the full integer type, even though there was no real support for that type. Peter and Aaron eventually solved this by adding support for opaque modes. However, Float128 needs to be a scalar floating point mode, not an opaque mode.
Hi! On Wed, Mar 03, 2021 at 02:12:56PM -0500, Michael Meissner wrote: > On Tue, Mar 02, 2021 at 03:53:06PM -0600, Segher Boessenkool wrote: > > If you want to make decimal and/or QP float work only on 64-bit LE Linux > > you should say so. And in that case, that is certainly not acceptable > > if it doesn't "sorry" at configure time already. > > Well in general the only supported configuration for _Float128 is 64-bit LE > Linux, but this is more due to the infrastructure not supporting it. > > If you want to support _Float128 on big endian, you need a GLIBC that provides > the necessary support. As far as I know, GLIBC does not build the _Float128 > support on big endian. This isn't the point. You make it harder to support in the future, for no reason (except a little convenience now). > You also need to set the minimum CPU to power7, because _Float128 is passed in > Altivec registers. Yes, another unnecessary big restriction. > As we have discussed many times, on 32-bit BE, you cannot use hardware > _Float128 support on power9/power10 because there is no TImode in 32-bit. That is a) another thing that needs fixing, and b) you *can* have floating point modes of a size that does not exist as integer mode. > Various machine independent parts of GCC require an integer type to be the same > size as basic types. And now explain float80 and float96? > With the current compiler on BE, if you use -mcpu=power7 or newer, it will > enable the _Float128/__float128 keywords, and generate the code. But until > there is an expectation of library support, it won't work for the general > user. So? That does not mean you can break it further! > > > Note the second issue would affect x86_64 cross compilers as well, since they > > > use those two functions to do the _Float128/Decimal versions. > > > > Yes, it cannot work correctly at all there, either. > > If you remember, the original versions of the patch would only work if you > configured the compiler to use GLIBC 2.32 or newer (such as using the > --with-advance-toolchain at14.0 option). You did not like this because as you > pointed out, you might use a different GLIBC when linking. It has nothing to do with me liking or not liking a patch. You are piling on more and more dependencies, when we cannot have any. *That* is the problem. And it is a *problem*. If we have a maze of limited situations that do and don't work, testing becomes impossible already, let alone all further support. Segher
diff --git a/libgcc/config/rs6000/_sprintfkf.c b/libgcc/config/rs6000/_sprintfkf.c index 2d624f14e25..9bbc26145ab 100644 --- a/libgcc/config/rs6000/_sprintfkf.c +++ b/libgcc/config/rs6000/_sprintfkf.c @@ -27,7 +27,6 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see #include <stdlib.h> #include <soft-fp.h> #include <quad-float128.h> -#include <stdio.h> #include <_sprintfkf.h> /* This function must be built with IBM 128-bit as long double, so that we can @@ -42,7 +41,11 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see If we are linked against an earlier library, we will have fake it by converting the value to long double, and using sprintf to do the conversion. This isn't ideal, as IEEE 128-bit has more exponent range than IBM - 128-bit. */ + 128-bit. + + We use __builtin_sprintf so that we don't have to include stdio.h to define + sprintf. Stdio.h might not be present for freestanding cross compilers that + do not need to include a library. */ extern int __sprintfieee128 (char *restrict, const char *restrict, ...) __attribute__ ((__weak__)); @@ -54,5 +57,5 @@ int __sprintfkf (char *restrict string, if (__sprintfieee128) return __sprintfieee128 (string, format, number); - return sprintf (string, format, (long double) number); + return __builtin_sprintf (string, format, (long double) number); }