| Message ID | 20210226063341.GA28298@ibm-toto.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
| Series | Honor --disable-decimal-float in building _Float128 support. | expand |
On Fri, 26 Feb 2021, Michael Meissner via Gcc-patches wrote:
> Joseph, does this patch fix your problem?
Yes, this fixes the build without libc done by build-many-glibcs.py.
On Fri, Feb 26, 2021 at 01:33:41AM -0500, Michael Meissner wrote: > Honor --disable-decimal-float in building _Float128 support. > > Joseph Myers reported that my previous patch to add conversions between > _Float128 and the Decimal types was still being built even if GCC was > configured with the --disable-decimal-float option. This patch fixes that by > only building the conversion functions if both _Float128 and Decimal support > are built into GCC. Okay. > In addition, I removed the dependency on the target having a valid stdio.h file > to declare sprintf in building _sprintfkf.c. In the future, do not do multiple independent things in one patch; this makes it harder than necessary to review. It also makes it harder for you to write good commit messages. With Git it is easy to break out, join, reorder, or transmogrify patches (see the -p option to various commands, for example). > Finally I noticed I had a mismatch in the _sprintfkf.h include file, and I > fixed that. What does this mean? A reviewer needs to know what the patch is supposed to do. You also should say that in the commit message (which ideally your mail message _is_ of course). > * config.host (powerpc*-*-linux): Move the Decimal/_Float128 > * conversions into t-float128-dec. Stray bullet. > * config/rs6000/t-float128 (fp128_dec_funcs): Move to > t-float128-dec. > (fp128_decstr_funcs): Move to t-float128-dec. > (ibm128_dec_funcs): Move to t-float128-dec. > (fp128_dec_objs): Move to t-float128-dec. > (fp128_decstr_objs): Move to t-float128-dec. > (ibm128_dec_objs): Move to t-float128-dec. > (FP128_CFLAGS_DECIMAL): Move to t-float128-dec. > (IBM128_CFLAGS_DECIMAL): Move to t-float128-dec. > * config/rs6000/t-float128-dec: New file. You don't say this in the mesasage, either. If you split code movements like this into a separate patch (at the start of the resulting series), it is *much* easier to review. Even more so if you made any changes to the code at the same time (not the case here, says the changelog). So, why did this move? I could try to find out, but you could just say, much less work :-) > * configure.ac (libgcc_cv_powerpc_float128_dec): New variable, set > to yes if both _Float128 and Decimal support are available. > * configure: Regenerate. Can't you just #ifdef the code? This is much harder to get (subtly) wrong. Also you then do not need a name for a complex subject, so you won't do a terrible name either. (Well, #if ENABLE_DECIMAL_FLOAT, it is defined to 0 instead of being undef'ed). Changelogs should not explain the code btw, do that in the code itself, or in the commit message. > -#include <stdio.h> > +extern int sprintf (char *restrict, const char *restrict, ...); I'm not sure that is an improvement at all. Why would it be? The point is not to not include standard headers (although, for free-standing you need that); the point is not to depend on external code, in libgcc. > -extern int __sprintfkf (char *restrict, const char *restrict, ...); > +extern int __sprintfkf (char *restrict, const char *restrict, _Float128); This is a separate bug fix, so should be separate. Segher
On Fri, Feb 26, 2021 at 04:36:20PM -0600, Segher Boessenkool wrote: > On Fri, Feb 26, 2021 at 01:33:41AM -0500, Michael Meissner wrote: > > Honor --disable-decimal-float in building _Float128 support. > > > > Joseph Myers reported that my previous patch to add conversions between > > _Float128 and the Decimal types was still being built even if GCC was > > configured with the --disable-decimal-float option. This patch fixes that by > > only building the conversion functions if both _Float128 and Decimal support > > are built into GCC. > > Okay. > > > In addition, I removed the dependency on the target having a valid stdio.h file > > to declare sprintf in building _sprintfkf.c. > > In the future, do not do multiple independent things in one patch; this > makes it harder than necessary to review. It also makes it harder for > you to write good commit messages. With Git it is easy to break out, > join, reorder, or transmogrify patches (see the -p option to various > commands, for example). Joseph reported three bugs in the report, so I implemented them in a single patch. > > Finally I noticed I had a mismatch in the _sprintfkf.h include file, and I > > fixed that. > > What does this mean? The include file _sprintfkf.h had a prototype of: extern int __sprintfkf (char *restrict, const char *restrict, ...); while the function in _sprintfkf.c actually was defined: int __sprintfkf (char *restrict string, const char *restrict format, _Float128 number) Since _sprintfkf.c did not include _sprintfkf.h, I didn't notice that these things were different. In fixing the bug to eliminate stdio.h, I noticed that we got a function declared without a previous declaration message, so I included the include file, and then noticed that the calling signature was different. > A reviewer needs to know what the patch is supposed to do. You also > should say that in the commit message (which ideally your mail message > _is_ of course). > > > * config.host (powerpc*-*-linux): Move the Decimal/_Float128 > > * conversions into t-float128-dec. > > Stray bullet. Thanks. > > * config/rs6000/t-float128 (fp128_dec_funcs): Move to > > t-float128-dec. > > (fp128_decstr_funcs): Move to t-float128-dec. > > (ibm128_dec_funcs): Move to t-float128-dec. > > (fp128_dec_objs): Move to t-float128-dec. > > (fp128_decstr_objs): Move to t-float128-dec. > > (ibm128_dec_objs): Move to t-float128-dec. > > (FP128_CFLAGS_DECIMAL): Move to t-float128-dec. > > (IBM128_CFLAGS_DECIMAL): Move to t-float128-dec. > > * config/rs6000/t-float128-dec: New file. > > You don't say this in the mesasage, either. > > If you split code movements like this into a separate patch (at the > start of the resulting series), it is *much* easier to review. Even > more so if you made any changes to the code at the same time (not the > case here, says the changelog). > > So, why did this move? I could try to find out, but you could just say, > much less work :-) > > > * configure.ac (libgcc_cv_powerpc_float128_dec): New variable, set > > to yes if both _Float128 and Decimal support are available. > > * configure: Regenerate. > > Can't you just #ifdef the code? This is much harder to get (subtly) > wrong. Also you then do not need a name for a complex subject, so you > won't do a terrible name either. (Well, #if ENABLE_DECIMAL_FLOAT, it > is defined to 0 instead of being undef'ed). Not really. The problem is these functions are buried within dfp-bit.c. You need to change the Makefile not to build these functions. I could use make ifs to suppress setting these variables. I tend to think having a separate t-* file is cleaner than a bug of make ifs within a single t-* file. > Changelogs should not explain the code btw, do that in the code itself, > or in the commit message. > > > -#include <stdio.h> > > > +extern int sprintf (char *restrict, const char *restrict, ...); > > I'm not sure that is an improvement at all. Why would it be? The point > is not to not include standard headers (although, for free-standing you > need that); the point is not to depend on external code, in libgcc. The idea is you want to be able to compile code that calls sprintf. If you don't have a stdio.h, the function will not compile. If it compiles, and is used, it will generate an error at link time. > > -extern int __sprintfkf (char *restrict, const char *restrict, ...); > > +extern int __sprintfkf (char *restrict, const char *restrict, _Float128); > > This is a separate bug fix, so should be separate.
On Fri, Feb 26, 2021 at 07:32:25PM -0500, Michael Meissner wrote: > On Fri, Feb 26, 2021 at 04:36:20PM -0600, Segher Boessenkool wrote: > > On Fri, Feb 26, 2021 at 01:33:41AM -0500, Michael Meissner wrote: > > > In addition, I removed the dependency on the target having a valid stdio.h file > > > to declare sprintf in building _sprintfkf.c. > > > > In the future, do not do multiple independent things in one patch; this > > makes it harder than necessary to review. It also makes it harder for > > you to write good commit messages. With Git it is easy to break out, > > join, reorder, or transmogrify patches (see the -p option to various > > commands, for example). > > Joseph reported three bugs in the report, so I implemented them in a single > patch. Just don't. Please. If you optimise your patches for reviewer time (and, assume the reviewer knows nothing about your patch yet), you'll end up with a nice series that is easy to write commit messages (and changelogs) for as well. Try it some time, you'll be amazed! > > > Finally I noticed I had a mismatch in the _sprintfkf.h include file, and I > > > fixed that. > > > > What does this mean? > > The include file _sprintfkf.h had a prototype of: > > extern int __sprintfkf (char *restrict, const char *restrict, ...); > > while the function in _sprintfkf.c actually was defined: > > int __sprintfkf (char *restrict string, > const char *restrict format, > _Float128 number) > > Since _sprintfkf.c did not include _sprintfkf.h, I didn't notice that these > things were different. Okay. This was clear after reading the patch, but the message should help reading the patch, not the other way around! > > > * configure.ac (libgcc_cv_powerpc_float128_dec): New variable, set > > > to yes if both _Float128 and Decimal support are available. > > > * configure: Regenerate. > > > > Can't you just #ifdef the code? This is much harder to get (subtly) > > wrong. Also you then do not need a name for a complex subject, so you > > won't do a terrible name either. (Well, #if ENABLE_DECIMAL_FLOAT, it > > is defined to 0 instead of being undef'ed). > > Not really. The problem is these functions are buried within dfp-bit.c. You > need to change the Makefile not to build these functions. I could use make ifs > to suppress setting these variables. I tend to think having a separate t-* > file is cleaner than a bug of make ifs within a single t-* file. I don't see how this is true. I cannot see what is new code and what is not anyway, so it is hard to look deeper. You can just do #if __FLOAT128__ && ENABLE_DECIMAL_FLOAT ... #endif (or similar, whichever macros you need exactly) around the code that cannot be built without both these things. > > > -#include <stdio.h> > > > > > +extern int sprintf (char *restrict, const char *restrict, ...); > > > > I'm not sure that is an improvement at all. Why would it be? The point > > is not to not include standard headers (although, for free-standing you > > need that); the point is not to depend on external code, in libgcc. > > The idea is you want to be able to compile code that calls sprintf. If you > don't have a stdio.h, the function will not compile. If it compiles, and is > used, it will generate an error at link time. If you have sprintf, you have <stdio.h>. The C standard demands it. Segher
diff --git a/libgcc/config.host b/libgcc/config.host index f808b61be70..b5b4b0b3ba0 100644 --- a/libgcc/config.host +++ b/libgcc/config.host @@ -1220,6 +1220,10 @@ powerpc*-*-linux*) tmake_file="${tmake_file} rs6000/t-float128" fi + if test $libgcc_cv_powerpc_float128_dec = yes; then + tmake_file="${tmake_file} rs6000/t-float128-dec" + fi + if test $libgcc_cv_powerpc_float128_hw = yes; then tmake_file="${tmake_file} rs6000/t-float128-hw" fi diff --git a/libgcc/config/rs6000/_sprintfkf.c b/libgcc/config/rs6000/_sprintfkf.c index a7fdfb483c9..b367861fcc3 100644 --- a/libgcc/config/rs6000/_sprintfkf.c +++ b/libgcc/config/rs6000/_sprintfkf.c @@ -27,7 +27,9 @@ 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> + +extern int sprintf (char *restrict, const char *restrict, ...); /* This function must be built with IBM 128-bit as long double, so that we can access the strfroml function if do not have an IEEE 128-bit version, and if diff --git a/libgcc/config/rs6000/_sprintfkf.h b/libgcc/config/rs6000/_sprintfkf.h index 637d104c882..22b245d2cbb 100644 --- a/libgcc/config/rs6000/_sprintfkf.h +++ b/libgcc/config/rs6000/_sprintfkf.h @@ -24,5 +24,5 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see /* Declaration of the conversion function to IEEE 128-bit floating point from string using snprintf. */ -extern int __sprintfkf (char *restrict, const char *restrict, ...); +extern int __sprintfkf (char *restrict, const char *restrict, _Float128); diff --git a/libgcc/config/rs6000/t-float128 b/libgcc/config/rs6000/t-float128 index 6fb1a3d871b..bbc66465050 100644 --- a/libgcc/config/rs6000/t-float128 +++ b/libgcc/config/rs6000/t-float128 @@ -22,18 +22,6 @@ fp128_softfp_static_obj = $(addsuffix -sw$(objext),$(fp128_softfp_funcs)) fp128_softfp_shared_obj = $(addsuffix -sw_s$(objext),$(fp128_softfp_funcs)) fp128_softfp_obj = $(fp128_softfp_static_obj) $(fp128_softfp_shared_obj) -# Decimal <-> _Float128 conversions -fp128_dec_funcs = _kf_to_sd _kf_to_dd _kf_to_td \ - _sd_to_kf _dd_to_kf _td_to_kf - -# _Float128 to/from string conversions that must be compiled with IBM 128-bit -# long double. -fp128_decstr_funcs = _strtokf _sprintfkf - -# Decimal <-> __ibm128 conversions -ibm128_dec_funcs = _tf_to_sd _tf_to_dd _tf_to_td \ - _sd_to_tf _dd_to_tf _td_to_tf - # New functions for software emulation fp128_ppc_funcs = floattikf floatuntikf fixkfti fixunskfti \ extendkftf2-sw trunctfkf2-sw \ @@ -48,13 +36,16 @@ fp128_ppc_obj = $(fp128_ppc_static_obj) $(fp128_ppc_shared_obj) # All functions fp128_funcs = $(fp128_softfp_funcs) $(fp128_ppc_funcs) \ - $(fp128_hw_funcs) $(fp128_ifunc_funcs) + $(fp128_hw_funcs) $(fp128_ifunc_funcs) \ + $(fp128_dec_funcs) fp128_src = $(fp128_softfp_src) $(fp128_ppc_src) \ - $(fp128_hw_src) $(fp128_ifunc_src) + $(fp128_hw_src) $(fp128_ifunc_src) \ + $(fp128_dec_src) fp128_obj = $(fp128_softfp_obj) $(fp128_ppc_obj) \ - $(fp128_hw_obj) $(fp128_ifunc_obj) + $(fp128_hw_obj) $(fp128_ifunc_obj) \ + $(fp128_dec_obj) fp128_sed = $(srcdir)/config/rs6000/float128-sed$(fp128_sed_hw) fp128_dep = $(fp128_sed) $(srcdir)/config/rs6000/t-float128 @@ -82,27 +73,6 @@ $(fp128_ppc_obj) : INTERNAL_CFLAGS += $(FP128_CFLAGS_SW) $(fp128_obj) : $(fp128_includes) $(fp128_obj) : $(srcdir)/config/rs6000/quad-float128.h -# Force the TF mode to/from decimal functions to be compiled with IBM long -# double. Add building the KF mode to/from decimal conversions with explict -# IEEE long double. -fp128_dec_objs = $(addsuffix $(objext),$(fp128_dec_funcs)) \ - $(addsuffix _s$(objext),$(fp128_dec_funcs)) - -fp128_decstr_objs = $(addsuffix $(objext),$(fp128_decstr_funcs)) \ - $(addsuffix _s$(objext),$(fp128_decstr_funcs)) - -ibm128_dec_objs = $(addsuffix $(objext),$(ibm128_dec_funcs)) \ - $(addsuffix _s$(objext),$(ibm128_dec_funcs)) - -FP128_CFLAGS_DECIMAL = -mno-gnu-attribute -Wno-psabi -mabi=ieeelongdouble -IBM128_CFLAGS_DECIMAL = -mno-gnu-attribute -Wno-psabi -mabi=ibmlongdouble - -$(fp128_dec_objs) : INTERNAL_CFLAGS += $(FP128_CFLAGS_DECIMAL) -$(fp128_decstr_objs) : INTERNAL_CFLAGS += $(IBM128_CFLAGS_DECIMAL) -$(ibm128_dec_objs) : INTERNAL_CFLAGS += $(IBM128_CFLAGS_DECIMAL) - -$(fp128_decstr_objs) : $(srcdir)/config/rs6000/_strtokf.h \ - $(srcdir)/config/rs6000/_sprintfkf.h \ $(fp128_softfp_src) : $(srcdir)/soft-fp/$(subst -sw,,$(subst kf,tf,$@)) $(fp128_dep) @src="$(srcdir)/soft-fp/$(subst -sw,,$(subst kf,tf,$@))"; \ diff --git a/libgcc/config/rs6000/t-float128-dec b/libgcc/config/rs6000/t-float128-dec new file mode 100644 index 00000000000..7a928260cec --- /dev/null +++ b/libgcc/config/rs6000/t-float128-dec @@ -0,0 +1,37 @@ +# Support for adding conversions between __float128 and Decimal to the powerpc. +# The t-float128 makefile fragment includes these objects if both float128 and +# decimal support is built-in. + +# Decimal <-> _Float128 conversions +fp128_dec_funcs = _kf_to_sd _kf_to_dd _kf_to_td \ + _sd_to_kf _dd_to_kf _td_to_kf + +# _Float128 to/from string conversions that must be compiled with IBM 128-bit +# long double. +fp128_decstr_funcs = _strtokf _sprintfkf + +# Decimal <-> __ibm128 conversions +ibm128_dec_funcs = _tf_to_sd _tf_to_dd _tf_to_td \ + _sd_to_tf _dd_to_tf _td_to_tf + +# Force the TF mode to/from decimal functions to be compiled with IBM long +# double. Add building the KF mode to/from decimal conversions with explict +# IEEE long double. +fp128_dec_objs = $(addsuffix $(objext),$(fp128_dec_funcs)) \ + $(addsuffix _s$(objext),$(fp128_dec_funcs)) + +fp128_decstr_objs = $(addsuffix $(objext),$(fp128_decstr_funcs)) \ + $(addsuffix _s$(objext),$(fp128_decstr_funcs)) + +ibm128_dec_objs = $(addsuffix $(objext),$(ibm128_dec_funcs)) \ + $(addsuffix _s$(objext),$(ibm128_dec_funcs)) + +FP128_CFLAGS_DECIMAL = $(FP128_CFLAGS_SW) -Wno-psabi -mabi=ieeelongdouble +IBM128_CFLAGS_DECIMAL = $(FP128_CFLAGS_SW) -Wno-psabi -mabi=ibmlongdouble + +$(fp128_dec_objs) : INTERNAL_CFLAGS += $(FP128_CFLAGS_DECIMAL) +$(fp128_decstr_objs) : INTERNAL_CFLAGS += $(IBM128_CFLAGS_DECIMAL) +$(ibm128_dec_objs) : INTERNAL_CFLAGS += $(IBM128_CFLAGS_DECIMAL) + +$(fp128_decstr_objs) : $(srcdir)/config/rs6000/_strtokf.h \ + $(srcdir)/config/rs6000/_sprintfkf.h \ diff --git a/libgcc/configure b/libgcc/configure index 78fc22a5784..0d8844e6d99 100755 --- a/libgcc/configure +++ b/libgcc/configure @@ -4913,7 +4913,7 @@ case "$host" in case "$enable_cet" in auto) # Check if target supports multi-byte NOPs - # and if assembler supports CET insn. + # and if compiler and assembler support CET insn. cet_save_CFLAGS="$CFLAGS" CFLAGS="$CFLAGS -fcf-protection" cat confdefs.h - <<_ACEOF >conftest.$ac_ext @@ -5228,6 +5228,26 @@ fi { $as_echo "$as_me:${as_lineno-$LINENO}: result: $libgcc_cv_powerpc_float128" >&5 $as_echo "$libgcc_cv_powerpc_float128" >&6; } + CFLAGS="$CFLAGS -mvsx -mfloat128" + { $as_echo "$as_me:${as_lineno-$LINENO}: checking for PowerPC __float128 libraries with decimal support" >&5 +$as_echo_n "checking for PowerPC __float128 libraries with decimal support... " >&6; } +if ${libgcc_cv_powerpc_float128_dec+:} false; then : + $as_echo_n "(cached) " >&6 +else + cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +_Float128 convert (_Decimal128 a) { return a; } +_ACEOF +if ac_fn_c_try_compile "$LINENO"; then : + libgcc_cv_powerpc_float128_dec=yes +else + libgcc_cv_powerpc_float128_dec=no +fi +rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $libgcc_cv_powerpc_float128_dec" >&5 +$as_echo "$libgcc_cv_powerpc_float128_dec" >&6; } + CFLAGS="$CFLAGS -mpower9-vector -mfloat128-hardware" { $as_echo "$as_me:${as_lineno-$LINENO}: checking for PowerPC ISA 3.0 to build hardware __float128 libraries" >&5 $as_echo_n "checking for PowerPC ISA 3.0 to build hardware __float128 libraries... " >&6; } diff --git a/libgcc/configure.ac b/libgcc/configure.ac index ed50c0e9b49..b5176265296 100644 --- a/libgcc/configure.ac +++ b/libgcc/configure.ac @@ -435,6 +435,14 @@ powerpc*-*-linux*) [libgcc_cv_powerpc_float128=yes], [libgcc_cv_powerpc_float128=no])]) + CFLAGS="$CFLAGS -mvsx -mfloat128" + AC_CACHE_CHECK([for PowerPC __float128 libraries with decimal support], + [libgcc_cv_powerpc_float128_dec], + [AC_COMPILE_IFELSE( + [AC_LANG_SOURCE([_Float128 convert (_Decimal128 a) { return a; }])], + [libgcc_cv_powerpc_float128_dec=yes], + [libgcc_cv_powerpc_float128_dec=no])]) + CFLAGS="$CFLAGS -mpower9-vector -mfloat128-hardware" AC_CACHE_CHECK([for PowerPC ISA 3.0 to build hardware __float128 libraries], [libgcc_cv_powerpc_float128_hw],