| Message ID | 32c02216-9f50-8b1b-9d5d-43a769c3c149@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | rs6000: Fix typos in float128 ISA3.1 support | expand |
Hi! On Mon, Jun 21, 2021 at 05:27:06PM +0800, Kewen.Lin wrote: > Recently if we build gcc on Power with the assembler which doesn't > have Power10 support, the build will fail when building libgcc with > one error message like: > > Error: invalid switch -mpower10 > Error: unrecognized option -mpower10 > make[2]: *** [...gcc/gcc-base/libgcc/shared-object.mk:14: float128-p10.o] Error 1 In general, it is recommended to use a binutils of approximately the same age as the GCC you are trying to build. This is similar to us not supporting most other non-sensical configurations. An important reason for that is it cannot ever be tested, there are just too many strange combinations possible. That said :-) > By checking the culprit commit r12-1340, it's caused by some typos. (That is 9090f4807161.) > - fix test case used for libgcc_cv_powerpc_3_1_float128_hw check. I was confused here for a bit, "test case" usually means something in testsuite/, I'd call this just "test" :-) > BTW, there are some noises during regression testings due to > newer versions binutils, but they were identified as unrelated > after some checkings. Hrm, what kind of noise? > * config/rs6000/t-float128-hw(fp128_3_1_hw_funcs, > fp128_3_1_hw_src, fp128_3_1_hw_static_obj, fp128_3_1_hw_shared_obj, > fp128_3_1_hw_obj): Remove variables for ISA 3.1 support. Needs a space before the opening paren. Doesn't need a line break so early on that line btw. Just "Remove." or "Delete." is less confusing btw: what you wrote can be read as "Remove the variables from these declarations" or similar. And of course terseness is usually best in a changelog. > * config/rs6000/t-float128-p10-hw (FLOAT128_HW_INSNS): Append > macro FLOAT128_HW_INSNS_ISA3_1 for ISA 3.1 support. Don't say what it is for, just say what changed :-) > (FP128_3_1_CFLAGS_HW): Fix option typo. > * config/rs6000/float128-ifunc.c (SW_OR_HW_ISA3_1): Guarded > with FLOAT128_HW_INSNS_ISA3_1. "Guard", not "Guarded", all entries are written in the imperative, like, "Do this" or "Guard that". > +#ifdef FLOAT128_HW_INSNS_ISA3_1 > TFtype __floattikf (TItype_ppc) > __attribute__ ((__ifunc__ ("__floattikf_resolve"))); I wonder if we now need TItype_ppc at all anymore, btw? Okay for trunk with the changelog slightly massaged. Thanks! Segher
Hi Segher, Thanks for the prompt review! on 2021/6/23 上午2:56, Segher Boessenkool wrote: > Hi! > > On Mon, Jun 21, 2021 at 05:27:06PM +0800, Kewen.Lin wrote: >> Recently if we build gcc on Power with the assembler which doesn't >> have Power10 support, the build will fail when building libgcc with >> one error message like: >> >> Error: invalid switch -mpower10 >> Error: unrecognized option -mpower10 >> make[2]: *** [...gcc/gcc-base/libgcc/shared-object.mk:14: float128-p10.o] Error 1 > > In general, it is recommended to use a binutils of approximately the > same age as the GCC you are trying to build. This is similar to us not > supporting most other non-sensical configurations. An important reason > for that is it cannot ever be tested, there are just too many strange > combinations possible. > > That said :-) > Ah, ok! It explained why no one reported this. I only used the customized binutils for Power10 build/testing before, will work with one new binutils from now on. It did have more testing coverage. >> By checking the culprit commit r12-1340, it's caused by some typos. > > (That is 9090f4807161.) > >> - fix test case used for libgcc_cv_powerpc_3_1_float128_hw check. > > I was confused here for a bit, "test case" usually means something in > testsuite/, I'd call this just "test" :-) > >> BTW, there are some noises during regression testings due to >> newer versions binutils, but they were identified as unrelated >> after some checkings. > > Hrm, what kind of noise? > Some check selectors will fail without new binutils, those cases becomes unsupported, like Power10 support checks. The location of our pre-built binutils in our system has not only binutils but also some other tools like newer version gdb, which caused some differences such as for guality cases. >> * config/rs6000/t-float128-hw(fp128_3_1_hw_funcs, >> fp128_3_1_hw_src, fp128_3_1_hw_static_obj, fp128_3_1_hw_shared_obj, >> fp128_3_1_hw_obj): Remove variables for ISA 3.1 support. > > Needs a space before the opening paren. Doesn't need a line break so > early on that line btw. > Good catch, fixed. > Just "Remove." or "Delete." is less confusing btw: what you wrote can be > read as "Remove the variables from these declarations" or similar. And > of course terseness is usually best in a changelog. > Fixed. >> * config/rs6000/t-float128-p10-hw (FLOAT128_HW_INSNS): Append >> macro FLOAT128_HW_INSNS_ISA3_1 for ISA 3.1 support. > > Don't say what it is for, just say what changed :-) > Fixed. >> (FP128_3_1_CFLAGS_HW): Fix option typo. >> * config/rs6000/float128-ifunc.c (SW_OR_HW_ISA3_1): Guarded >> with FLOAT128_HW_INSNS_ISA3_1. > > "Guard", not "Guarded", all entries are written in the imperative, like, > "Do this" or "Guard that". > Got it, fixed. >> +#ifdef FLOAT128_HW_INSNS_ISA3_1 >> TFtype __floattikf (TItype_ppc) >> __attribute__ ((__ifunc__ ("__floattikf_resolve"))); > > I wonder if we now need TItype_ppc at all anymore, btw? > Sorry that I don't quite follow this question. I think it stands for type signed int128, the function is to convert signed int128 to float128, it would be needed here. But I think that's not what you asked. Or you are referring to replace this type with signed int128 without shielding it with mode? If yes, it sounds like a question for Mike. > Okay for trunk with the changelog slightly massaged. Thanks! > Thanks for catching changelog problems and the fix suggestion! Fixed all of them accordingly and committed in r12-1738. BR, Kewen
On Wed, Jun 23, 2021 at 12:17:07PM +0800, Kewen.Lin wrote: > >> +#ifdef FLOAT128_HW_INSNS_ISA3_1 > >> TFtype __floattikf (TItype_ppc) > >> __attribute__ ((__ifunc__ ("__floattikf_resolve"))); > > > > I wonder if we now need TItype_ppc at all anymore, btw? > > Sorry that I don't quite follow this question. I thought it may do the same as just TItype now, but the ifunc stuff probably makes it different still :-) Segher
on 2021/6/24 上午12:58, Segher Boessenkool wrote: > On Wed, Jun 23, 2021 at 12:17:07PM +0800, Kewen.Lin wrote: >>>> +#ifdef FLOAT128_HW_INSNS_ISA3_1 >>>> TFtype __floattikf (TItype_ppc) >>>> __attribute__ ((__ifunc__ ("__floattikf_resolve"))); >>> >>> I wonder if we now need TItype_ppc at all anymore, btw? >> >> Sorry that I don't quite follow this question. > > I thought it may do the same as just TItype now, but the ifunc stuff > probably makes it different still :-) > Ah, thanks for the clarification! If I read it right, TItype is defined with __attribute__ ((mode (TI))) while TItype_ppc is defined with __attribute__ ((__mode__ (__TI__))), the later writing looks special. BR, Kewen
On Thu, Jun 24, 2021 at 05:32:20PM +0800, Kewen.Lin wrote: > on 2021/6/24 上午12:58, Segher Boessenkool wrote: > > On Wed, Jun 23, 2021 at 12:17:07PM +0800, Kewen.Lin wrote: > >>>> +#ifdef FLOAT128_HW_INSNS_ISA3_1 > >>>> TFtype __floattikf (TItype_ppc) > >>>> __attribute__ ((__ifunc__ ("__floattikf_resolve"))); > >>> > >>> I wonder if we now need TItype_ppc at all anymore, btw? > >> > >> Sorry that I don't quite follow this question. > > > > I thought it may do the same as just TItype now, but the ifunc stuff > > probably makes it different still :-) > > Ah, thanks for the clarification! If I read it right, TItype is defined > with __attribute__ ((mode (TI))) while TItype_ppc is defined with > __attribute__ ((__mode__ (__TI__))), the later writing looks special. I managed to read things wrong, I thought there was some ifunc stuff in the definition of TItype_ppc. Of course there is not, it is just setting the mode. mode(__TI__) is just the more portable way of writing mode(TI), the latter will not work if something #define's TI (you cannot do that with __TI__, you are not allowed to by the C standard, in application code). So it looks like we could just use {U,}TItype here, no _ppc. Carl, is there some reason I'm not seeing you used it? Segher
on 2021/6/25 上午3:36, Segher Boessenkool wrote: > On Thu, Jun 24, 2021 at 05:32:20PM +0800, Kewen.Lin wrote: >> on 2021/6/24 上午12:58, Segher Boessenkool wrote: >>> On Wed, Jun 23, 2021 at 12:17:07PM +0800, Kewen.Lin wrote: >>>>>> +#ifdef FLOAT128_HW_INSNS_ISA3_1 >>>>>> TFtype __floattikf (TItype_ppc) >>>>>> __attribute__ ((__ifunc__ ("__floattikf_resolve"))); >>>>> >>>>> I wonder if we now need TItype_ppc at all anymore, btw? >>>> >>>> Sorry that I don't quite follow this question. >>> >>> I thought it may do the same as just TItype now, but the ifunc stuff >>> probably makes it different still :-) >> >> Ah, thanks for the clarification! If I read it right, TItype is defined >> with __attribute__ ((mode (TI))) while TItype_ppc is defined with >> __attribute__ ((__mode__ (__TI__))), the later writing looks special. > > I managed to read things wrong, I thought there was some ifunc stuff in > the definition of TItype_ppc. Of course there is not, it is just > setting the mode. > > mode(__TI__) is just the more portable way of writing mode(TI), the > latter will not work if something #define's TI (you cannot do that with > __TI__, you are not allowed to by the C standard, in application code). > Yeah, thanks for the note. It looks better to update the generic macro with this ppc style "__" writting and remove ppc one. :-) One related bug PR101235 was just opened, I noticed the culprit commit was backported to GCC11, is it OK to backport this fix to GCC 11 if everything goes well in one more week? BR, Kewen
On Mon, Jun 28, 2021 at 04:15:15PM +0800, Kewen.Lin wrote: > on 2021/6/25 上午3:36, Segher Boessenkool wrote: > > mode(__TI__) is just the more portable way of writing mode(TI), the > > latter will not work if something #define's TI (you cannot do that with > > __TI__, you are not allowed to by the C standard, in application code). > > Yeah, thanks for the note. It looks better to update the generic > macro with this ppc style "__" writting and remove ppc one. :-) > > One related bug PR101235 was just opened, I noticed the culprit commit > was backported to GCC11, is it OK to backport this fix to GCC 11 if > everything goes well in one more week? Please backport this immediately. Thanks! Segher
diff --git a/libgcc/config/rs6000/float128-ifunc.c b/libgcc/config/rs6000/float128-ifunc.c index 57545dd7edb..ef7f731bf0b 100644 --- a/libgcc/config/rs6000/float128-ifunc.c +++ b/libgcc/config/rs6000/float128-ifunc.c @@ -46,7 +46,9 @@ #endif #define SW_OR_HW(SW, HW) (__builtin_cpu_supports ("ieee128") ? HW : SW) +#ifdef FLOAT128_HW_INSNS_ISA3_1 #define SW_OR_HW_ISA3_1(SW, HW) (__builtin_cpu_supports ("arch_3_1") ? HW : SW) +#endif /* Resolvers. */ static __typeof__ (__addkf3_sw) * @@ -97,6 +99,7 @@ __floatdikf_resolve (void) return SW_OR_HW (__floatdikf_sw, __floatdikf_hw); } +#ifdef FLOAT128_HW_INSNS_ISA3_1 static __typeof__ (__floattikf_sw) * __floattikf_resolve (void) { @@ -108,6 +111,7 @@ __floatuntikf_resolve (void) { return SW_OR_HW_ISA3_1 (__floatuntikf_sw, __floatuntikf_hw); } +#endif static __typeof__ (__floatunsikf_sw) * __floatunsikf_resolve (void) @@ -121,7 +125,7 @@ __floatundikf_resolve (void) return SW_OR_HW (__floatundikf_sw, __floatundikf_hw); } - +#ifdef FLOAT128_HW_INSNS_ISA3_1 static __typeof__ (__fixkfti_sw) * __fixkfti_resolve (void) { @@ -133,6 +137,7 @@ __fixunskfti_resolve (void) { return SW_OR_HW_ISA3_1 (__fixunskfti_sw, __fixunskfti_hw); } +#endif static __typeof__ (__fixkfsi_sw) * __fixkfsi_resolve (void) @@ -323,6 +328,7 @@ TFtype __floatsikf (SItype_ppc) TFtype __floatdikf (DItype_ppc) __attribute__ ((__ifunc__ ("__floatdikf_resolve"))); +#ifdef FLOAT128_HW_INSNS_ISA3_1 TFtype __floattikf (TItype_ppc) __attribute__ ((__ifunc__ ("__floattikf_resolve"))); @@ -334,6 +340,7 @@ TItype_ppc __fixkfti (TFtype) UTItype_ppc __fixunskfti (TFtype) __attribute__ ((__ifunc__ ("__fixunskfti_resolve"))); +#endif TFtype __floatunsikf (USItype_ppc) __attribute__ ((__ifunc__ ("__floatunsikf_resolve"))); diff --git a/libgcc/config/rs6000/t-float128-hw b/libgcc/config/rs6000/t-float128-hw index c0827366cc4..d64ca4dd694 100644 --- a/libgcc/config/rs6000/t-float128-hw +++ b/libgcc/config/rs6000/t-float128-hw @@ -13,13 +13,6 @@ fp128_hw_static_obj = $(addsuffix $(objext),$(fp128_hw_funcs)) fp128_hw_shared_obj = $(addsuffix _s$(objext),$(fp128_hw_funcs)) fp128_hw_obj = $(fp128_hw_static_obj) $(fp128_hw_shared_obj) -# New functions for ISA 3.1 hardware support -fp128_3_1_hw_funcs = float128-p10 -fp128_3_1_hw_src = $(srcdir)/config/rs6000/float128-p10.c -fp128_3_1_hw_static_obj = $(addsuffix $(objext),$(fp128_3_1_hw_funcs)) -fp128_3_1_hw_shared_obj = $(addsuffix _s$(objext),$(fp128_3_1_hw_funcs)) -fp128_3_1_hw_obj = $(fp128_3_1_hw_static_obj) $(fp128_3_1_hw_shared_obj) - fp128_ifunc_funcs = float128-ifunc fp128_ifunc_src = $(srcdir)/config/rs6000/float128-ifunc.c fp128_ifunc_static_obj = float128-ifunc$(objext) @@ -37,18 +30,9 @@ FP128_CFLAGS_HW = -Wno-type-limits -mvsx -mfloat128 \ -I$(srcdir)/config/rs6000 \ $(FLOAT128_HW_INSNS) -FP128_3_1_CFLAGS_HW = -Wno-type-limits -mvsx -mcpu=power10 \ - -mfloat128-hardware -mno-gnu-attribute \ - -I$(srcdir)/soft-fp \ - -I$(srcdir)/config/rs6000 \ - $(FLOAT128_HW_INSNS) - $(fp128_hw_obj) : INTERNAL_CFLAGS += $(FP128_CFLAGS_HW) $(fp128_hw_obj) : $(srcdir)/config/rs6000/t-float128-hw -$(fp128_3_1_hw_obj) : INTERNAL_CFLAGS += $(FP128_3_1_CFLAGS_HW) -$(fp128_3_1_hw_obj) : $(srcdir)/config/rs6000/t-float128-p10-hw - $(fp128_ifunc_obj) : INTERNAL_CFLAGS += $(FP128_CFLAGS_SW) $(fp128_ifunc_obj) : $(srcdir)/config/rs6000/t-float128-hw diff --git a/libgcc/config/rs6000/t-float128-p10-hw b/libgcc/config/rs6000/t-float128-p10-hw index de36227c3d1..edaaee0e478 100644 --- a/libgcc/config/rs6000/t-float128-p10-hw +++ b/libgcc/config/rs6000/t-float128-p10-hw @@ -2,7 +2,7 @@ # Tell the float128 functions that the ISA 3.1 hardware support can # be compiled it to be selected via IFUNC functions. -FLOAT128_HW_INSNS = -DFLOAT128_HW_INSNS +FLOAT128_HW_INSNS += -DFLOAT128_HW_INSNS_ISA3_1 # New functions for hardware support @@ -14,7 +14,7 @@ fp128_3_1_hw_obj = $(fp128_3_1_hw_static_obj) $(fp128_3_1_hw_shared_obj) # Build the hardware support functions with appropriate hardware support FP128_3_1_CFLAGS_HW = -Wno-type-limits -mvsx -mfloat128 \ - -mpower10 \ + -mcpu=power10 \ -mfloat128-hardware -mno-gnu-attribute \ -I$(srcdir)/soft-fp \ -I$(srcdir)/config/rs6000 \ diff --git a/libgcc/configure b/libgcc/configure index ce05e0dd48b..4919a56f518 100755 --- a/libgcc/configure +++ b/libgcc/configure @@ -5265,10 +5265,10 @@ $as_echo "$libgcc_cv_powerpc_float128_hw" >&6; } CFLAGS="$saved_CFLAGS" saved_CFLAGS="$CFLAGS" - CFLAGS="$CFLAGS -mpower10 -mfloat128-hardware" + CFLAGS="$CFLAGS -mcpu=power10 -mfloat128-hardware" { $as_echo "$as_me:${as_lineno-$LINENO}: checking for PowerPC ISA 3.1 to build hardware __float128 libraries" >&5 $as_echo_n "checking for PowerPC ISA 3.1 to build hardware __float128 libraries... " >&6; } -if ${libgcc_cv_powerpc_float128_hw+:} false; then : +if ${libgcc_cv_powerpc_3_1_float128_hw+:} false; then : $as_echo_n "(cached) " >&6 else cat confdefs.h - <<_ACEOF >conftest.$ac_ext @@ -5280,15 +5280,15 @@ else #ifndef __BUILTIN_CPU_SUPPORTS__ #error "__builtin_cpu_supports is not available" #endif - vector unsigned char add (vector unsigned char a, vector unsigned char b) + vector unsigned char conv (vector unsigned char qs) { vector unsigned char ret; - __asm__ ("xscvsqqp %0,%1,%2" : "=v" (ret) : "v" (a), "v" (b)); + __asm__ ("xscvsqqp %0,%1" : "=v" (ret) : "v" (qs)); return ret; } - void *add_resolver (void) { return (void *) add; } - __float128 add_ifunc (__float128, __float128) - __attribute__ ((__ifunc__ ("add_resolver"))); + void *conv_resolver (void) { return (void *) conv; } + __float128 conv_ifunc (__float128) + __attribute__ ((__ifunc__ ("conv_resolver"))); _ACEOF if ac_fn_c_try_compile "$LINENO"; then : libgcc_cv_powerpc_3_1_float128_hw=yes @@ -5297,8 +5297,8 @@ else 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_hw" >&5 - $as_echo "$libgcc_cv_powerpc_float128_hw" >&6; } +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $libgcc_cv_powerpc_3_1_float128_hw" >&5 +$as_echo "$libgcc_cv_powerpc_3_1_float128_hw" >&6; } CFLAGS="$saved_CFLAGS" esac diff --git a/libgcc/configure.ac b/libgcc/configure.ac index bc315dec7e4..13a80b2551b 100644 --- a/libgcc/configure.ac +++ b/libgcc/configure.ac @@ -460,9 +460,9 @@ powerpc*-*-linux*) CFLAGS="$saved_CFLAGS" saved_CFLAGS="$CFLAGS" - CFLAGS="$CFLAGS -mpower10 -mfloat128-hardware" + CFLAGS="$CFLAGS -mcpu=power10 -mfloat128-hardware" AC_CACHE_CHECK([for PowerPC ISA 3.1 to build hardware __float128 libraries], - [libgcc_cv_powerpc_float128_hw], + [libgcc_cv_powerpc_3_1_float128_hw], [AC_COMPILE_IFELSE( [AC_LANG_SOURCE([#include <sys/auxv.h> #ifndef AT_PLATFORM @@ -471,15 +471,15 @@ powerpc*-*-linux*) #ifndef __BUILTIN_CPU_SUPPORTS__ #error "__builtin_cpu_supports is not available" #endif - vector unsigned char add (vector unsigned char a, vector unsigned char b) + vector unsigned char conv (vector unsigned char qs) { vector unsigned char ret; - __asm__ ("xscvsqqp %0,%1,%2" : "=v" (ret) : "v" (a), "v" (b)); + __asm__ ("xscvsqqp %0,%1" : "=v" (ret) : "v" (qs)); return ret; } - void *add_resolver (void) { return (void *) add; } - __float128 add_ifunc (__float128, __float128) - __attribute__ ((__ifunc__ ("add_resolver")));])], + void *conv_resolver (void) { return (void *) conv; } + __float128 conv_ifunc (__float128) + __attribute__ ((__ifunc__ ("conv_resolver")));])], [libgcc_cv_powerpc_3_1_float128_hw=yes], [libgcc_cv_powerpc_3_1_float128_hw=no])]) CFLAGS="$saved_CFLAGS"
Hi, Recently if we build gcc on Power with the assembler which doesn't have Power10 support, the build will fail when building libgcc with one error message like: Error: invalid switch -mpower10 Error: unrecognized option -mpower10 make[2]: *** [...gcc/gcc-base/libgcc/shared-object.mk:14: float128-p10.o] Error 1 By checking the culprit commit r12-1340, it's caused by some typos. What the proposed patch does: - fix test target typo libgcc_cv_powerpc_3_1_float128_hw (written wrongly as libgcc_cv_powerpc_float128_hw, so it's going to build ISA3.1 stuffs just when detecting ISA3.0). - fix test case used for libgcc_cv_powerpc_3_1_float128_hw check. - fix test option used for libgcc_cv_powerpc_3_1_float128_hw check. - remove the ISA3.1 related contents from t-float128-hw. - add new macro FLOAT128_HW_INSNS_ISA3_1 to differentiate ISA3.1 content from ISA3.0 part in ifunc support. For the last two points, I think it's by design, when the assembler only supports power9 insns but not power10 insns, we are unable to build power10 hw supported function successfully, so we shouldn't generate related ifunc stuffs which rely on p10 insns. This patch makes the build happy even without p10 supported as. Bootstrapped/regtested on: - powerpc64le-linux-gnu P10 - powerpc64le-linux-gnu P9 (w/i and w/o p10 supported as) - powerpc64-linux-gnu P8 (w/i and w/o p10 supported as) BTW, there are some noises during regression testings due to newer versions binutils, but they were identified as unrelated after some checkings. Is it ok for trunk? BR, Kewen ----- libgcc/ChangeLog: * configure: Regenerate. * configure.ac (test for libgcc_cv_powerpc_3_1_float128_hw): Fix typos among name, CFLAGS and test case. * config/rs6000/t-float128-hw(fp128_3_1_hw_funcs, fp128_3_1_hw_src, fp128_3_1_hw_static_obj, fp128_3_1_hw_shared_obj, fp128_3_1_hw_obj): Remove variables for ISA 3.1 support. * config/rs6000/t-float128-p10-hw (FLOAT128_HW_INSNS): Append macro FLOAT128_HW_INSNS_ISA3_1 for ISA 3.1 support. (FP128_3_1_CFLAGS_HW): Fix option typo. * config/rs6000/float128-ifunc.c (SW_OR_HW_ISA3_1): Guarded with FLOAT128_HW_INSNS_ISA3_1. (__floattikf_resolve): Likewise. (__floatuntikf_resolve): Likewise. (__fixkfti_resolve): Likewise. (__fixunskfti_resolve): Likewise. (__floattikf): Likewise. (__floatuntikf): Likewise. (__fixkfti): Likewise. (__fixunskfti): Likewise. libgcc/config/rs6000/float128-ifunc.c | 9 ++++++++- libgcc/config/rs6000/t-float128-hw | 16 ---------------- libgcc/config/rs6000/t-float128-p10-hw | 4 ++-- libgcc/configure | 18 +++++++++--------- libgcc/configure.ac | 14 +++++++------- 5 files changed, 26 insertions(+), 35 deletions(-)