| Message ID | 20180613211015.GA3737@ibm-toto.the-meissners.org |
|---|---|
| State | New |
| Headers | show |
| Series | , PowerPC long double transistion, patch #1 | expand |
In addition to the previous patch to aid in transitioning the PowerPC long double format to IEEE 128-bit, I have some additional patches that are needed. The previous patch is: https://gcc.gnu.org/ml/gcc-patches/2018-06/msg00634.html This patch prevents re-running the ibm128 and ieee128 initialization functions when the compiler was handling the clone and target attributes/pragmas. In particular, the compiler would issue an internal compiler error or segfault when the complex multiply/divide built-in functions were re-defined. Note, the GLIBC library that I used is not fully converted. I'm ignoring the issues that are due to the library, and just focusing on the compiler support. I have done separate bootstraps on a little endian power8 system with the long double type set to IBM extended and IEEE 128-bit extended. There were no regressions in using this patch. Can I check it in, and eventually back port it to GCC 8.2 with the other long double transition patches. 2018-06-13 Michael Meissner <meissner@linux.ibm.com> * config/rs6000/rs6000.c (rs6000_init_libfuncs): Do not re-run the initialization during clone or target pragma/attribute processing.
In addition to the previous patch to aid in transitioning the PowerPC long double format to IEEE 128-bit, I have some additional patches that are needed. The previous patch is: https://gcc.gnu.org/ml/gcc-patches/2018-06/msg00634.html This patch fixes the power8 implementation of copysign for IEEE 128-bit floating point. In particular, the way the temporary register was allocated did not use the normal GCC conventions of using a clobber with match_scratch. Because the constraint did not include a '&', the temporary register could have used one of the input registers. Note, the GLIBC library that I used is not fully converted. I'm ignoring the issues that are due to the library, and just focusing on the compiler support. I have done separate bootstraps on a little endian power8 system with the long double type set to IBM extended and IEEE 128-bit extended. There were no regressions in using this patch. Can I check it in, and eventually back port it to GCC 8.2 with the other long double transition patches. 2018-06-13 Michael Meissner <meissner@linux.ibm.com> * config/rs6000/rs6000.md (copysign<mode>3, IEEE iterator): Rework copysign of float128 on ISA 2.07 to use an explicit clobber, instead of passing in a temporary. (copysign<mode>3_soft): Likewise.
On Wed, Jun 13, 2018 at 05:21:55PM -0400, Michael Meissner wrote: > In addition to the previous patch to aid in transitioning the PowerPC long > double format to IEEE 128-bit, I have some additional patches that are needed. > The previous patch is: > https://gcc.gnu.org/ml/gcc-patches/2018-06/msg00634.html > > This patch prevents re-running the ibm128 and ieee128 initialization functions > when the compiler was handling the clone and target attributes/pragmas. In > particular, the compiler would issue an internal compiler error or segfault > when the complex multiply/divide built-in functions were re-defined. > > Note, the GLIBC library that I used is not fully converted. I'm ignoring the > issues that are due to the library, and just focusing on the compiler support. > > I have done separate bootstraps on a little endian power8 system with the long > double type set to IBM extended and IEEE 128-bit extended. There were no > regressions in using this patch. Can I check it in, and eventually back port > it to GCC 8.2 with the other long double transition patches. With the various library failures, I didn't notice that this patch does break LTO. So I will need to rework this patch so that it fixes both LTO and clone/target attributes.
On Wed, Jun 13, 2018 at 05:10:15PM -0400, Michael Meissner wrote: > In addition to the previous patch to aid in transitioning the PowerPC long > double format to IEEE 128-bit, I have some additional patches that are needed. > The previous patch is: > https://gcc.gnu.org/ml/gcc-patches/2018-06/msg00634.html > > This patch turns off setting the .gnu_attributes when building the libgcc > helper functions. I ran into some linker warnings as I built some multilib > toolchains, and this turns off those warnings. Should we turn it off for all of libgcc instead? > I have done separate bootstraps on a little endian power8 system with the long > double type set to IBM extended and IEEE 128-bit extended. There were no > regressions in using this patch. Can I check it in, and eventually back port > it to GCC 8.2 with the other long double transition patches? > > 2018-06-11 Michael Meissner <meissner@linux.ibm.com> > > * config/rs6000/t-float128 (FP128_CFLAGS_SW): Compile float128 > support modules with -mno-gnu-attribute. > * config/rs6000/t-float128-hw (FP128_CFLAGS_HW): Likewise. The patch is okay for trunk with or without such a change (but please investigate). All these 128-bit FP patches are okay for 8.2 too, after some simmering etc. Segher
On Thu, Jun 14, 2018 at 06:25:49PM -0500, Segher Boessenkool wrote: > On Wed, Jun 13, 2018 at 05:10:15PM -0400, Michael Meissner wrote: > > In addition to the previous patch to aid in transitioning the PowerPC long > > double format to IEEE 128-bit, I have some additional patches that are needed. > > The previous patch is: > > https://gcc.gnu.org/ml/gcc-patches/2018-06/msg00634.html > > > > This patch turns off setting the .gnu_attributes when building the libgcc > > helper functions. I ran into some linker warnings as I built some multilib > > toolchains, and this turns off those warnings. > > Should we turn it off for all of libgcc instead? It is only needed if long double is passed or returned. The conversion and support routines get called with long double values if long double is IEEE 128-bit, and with KFmode values if not. Any other libgcc function that has a long double interface should get the linker warning that the wrong type was used. > > I have done separate bootstraps on a little endian power8 system with the long > > double type set to IBM extended and IEEE 128-bit extended. There were no > > regressions in using this patch. Can I check it in, and eventually back port > > it to GCC 8.2 with the other long double transition patches? > > > > 2018-06-11 Michael Meissner <meissner@linux.ibm.com> > > > > * config/rs6000/t-float128 (FP128_CFLAGS_SW): Compile float128 > > support modules with -mno-gnu-attribute. > > * config/rs6000/t-float128-hw (FP128_CFLAGS_HW): Likewise. > > The patch is okay for trunk with or without such a change (but please > investigate). All these 128-bit FP patches are okay for 8.2 too, after > some simmering etc.
On Wed, Jun 13, 2018 at 05:25:33PM -0400, Michael Meissner wrote: > This patch fixes the power8 implementation of copysign for IEEE 128-bit > floating point. In particular, the way the temporary register was allocated > did not use the normal GCC conventions of using a clobber with match_scratch. > Because the constraint did not include a '&', the temporary register could have > used one of the input registers. > --- gcc/config/rs6000/rs6000.md (revision 261512) > +++ gcc/config/rs6000/rs6000.md (working copy) > @@ -14102,11 +14102,8 @@ (define_expand "copysign<mode>3" > emit_insn (gen_copysign<mode>3_hard (operands[0], operands[1], > operands[2])); > else > - { > - rtx tmp = gen_reg_rtx (<MODE>mode); > - emit_insn (gen_copysign<mode>3_soft (operands[0], operands[1], > - operands[2], tmp)); > - } > + emit_insn (gen_copysign<mode>3_soft (operands[0], operands[1], > + operands[2])); > DONE; > }) > > @@ -14125,9 +14122,9 @@ (define_insn "copysign<mode>3_soft" > [(set (match_operand:IEEE128 0 "altivec_register_operand" "=v") > (unspec:IEEE128 > [(match_operand:IEEE128 1 "altivec_register_operand" "v") > - (match_operand:IEEE128 2 "altivec_register_operand" "v") > - (match_operand:IEEE128 3 "altivec_register_operand" "+v")] > - UNSPEC_COPYSIGN))] > + (match_operand:IEEE128 2 "altivec_register_operand" "v")] > + UNSPEC_COPYSIGN)) > + (clobber (match_scratch:IEEE128 3 "=&v"))] > "!TARGET_FLOAT128_HW && FLOAT128_IEEE_P (<MODE>mode)" > "xscpsgndp %x3,%x2,%x1\;xxpermdi %x0,%x3,%x1,1" > [(set_attr "type" "veccomplex") Does this have to be two machine insns for one RTL insns? If you split it in the expander you don't need the clobber or the earlyclobber or anything like that. Okay if splitting it leads to problems elsewhere. Segher
On Fri, Jun 15, 2018 at 01:39:49PM -0500, Segher Boessenkool wrote: > On Wed, Jun 13, 2018 at 05:25:33PM -0400, Michael Meissner wrote: > > This patch fixes the power8 implementation of copysign for IEEE 128-bit > > floating point. In particular, the way the temporary register was allocated > > did not use the normal GCC conventions of using a clobber with match_scratch. > > Because the constraint did not include a '&', the temporary register could have > > used one of the input registers. > > > --- gcc/config/rs6000/rs6000.md (revision 261512) > > +++ gcc/config/rs6000/rs6000.md (working copy) > > @@ -14102,11 +14102,8 @@ (define_expand "copysign<mode>3" > > emit_insn (gen_copysign<mode>3_hard (operands[0], operands[1], > > operands[2])); > > else > > - { > > - rtx tmp = gen_reg_rtx (<MODE>mode); > > - emit_insn (gen_copysign<mode>3_soft (operands[0], operands[1], > > - operands[2], tmp)); > > - } > > + emit_insn (gen_copysign<mode>3_soft (operands[0], operands[1], > > + operands[2])); > > DONE; > > }) > > > > @@ -14125,9 +14122,9 @@ (define_insn "copysign<mode>3_soft" > > [(set (match_operand:IEEE128 0 "altivec_register_operand" "=v") > > (unspec:IEEE128 > > [(match_operand:IEEE128 1 "altivec_register_operand" "v") > > - (match_operand:IEEE128 2 "altivec_register_operand" "v") > > - (match_operand:IEEE128 3 "altivec_register_operand" "+v")] > > - UNSPEC_COPYSIGN))] > > + (match_operand:IEEE128 2 "altivec_register_operand" "v")] > > + UNSPEC_COPYSIGN)) > > + (clobber (match_scratch:IEEE128 3 "=&v"))] > > "!TARGET_FLOAT128_HW && FLOAT128_IEEE_P (<MODE>mode)" > > "xscpsgndp %x3,%x2,%x1\;xxpermdi %x0,%x3,%x1,1" > > [(set_attr "type" "veccomplex") > > Does this have to be two machine insns for one RTL insns? If you split it > in the expander you don't need the clobber or the earlyclobber or anything > like that. > > Okay if splitting it leads to problems elsewhere. I suspect it cannot be split before register allocation, just because that has been a source of problems in the past.
On Thu, 14 Jun 2018, Michael Meissner wrote: > Any other libgcc function that has a long double interface should get the > linker warning that the wrong type was used. libgcc functions have interfaces corresponding to a particular machine mode and names that vary depending on that mode - not interfaces directly involving a type such as long double. The interfaces for ibm128 format are generally *tf* (or __gcc_q*) and those for ieee128 format are generally *kf*. As far as I know, all the previously observed issues with some functions getting built for the wrong format have by now been fixed, and if any more such issues arise in future, they are simply libgcc bugs to be fixed in libgcc. Because the names depend on the format not the type, there is no risk of ABI inconsistency from linking with a libgcc function for the wrong format, and so I think the linker warning is never relevant for linking with libgcc (and once the glibc support is in, it won't be relevant for linking with glibc either). Linker warnings are relevant for the generic case of a library that *doesn't* do anything special to support both formats simultaneously - not for a library that properly handles getting things right for both formats automatically.
On Thu, Jun 14, 2018 at 08:18:42PM -0400, Michael Meissner wrote: > On Thu, Jun 14, 2018 at 06:25:49PM -0500, Segher Boessenkool wrote: > > On Wed, Jun 13, 2018 at 05:10:15PM -0400, Michael Meissner wrote: > > > In addition to the previous patch to aid in transitioning the PowerPC long > > > double format to IEEE 128-bit, I have some additional patches that are needed. > > > The previous patch is: > > > https://gcc.gnu.org/ml/gcc-patches/2018-06/msg00634.html > > > > > > This patch turns off setting the .gnu_attributes when building the libgcc > > > helper functions. I ran into some linker warnings as I built some multilib > > > toolchains, and this turns off those warnings. > > > > Should we turn it off for all of libgcc instead? > > It is only needed if long double is passed or returned. The conversion and > support routines get called with long double values if long double is IEEE > 128-bit, and with KFmode values if not. > > Any other libgcc function that has a long double interface should get the > linker warning that the wrong type was used. I don't get this. We have separate function names for all functions working on different long double types. The compiler knows what type is in use, so it won't generate a call to the wrong function. Or if it will, that is no different from calling the wrong define_insn, etc., so the attributes are not a good seat belt system anyway. I don't see how gnu_attributes ever helps in libgcc (not just for long double: in general). Segher
On Fri, Jun 15, 2018 at 03:56:57PM -0400, Michael Meissner wrote: > On Fri, Jun 15, 2018 at 01:39:49PM -0500, Segher Boessenkool wrote: > > On Wed, Jun 13, 2018 at 05:25:33PM -0400, Michael Meissner wrote: > > > This patch fixes the power8 implementation of copysign for IEEE 128-bit > > > floating point. In particular, the way the temporary register was allocated > > > did not use the normal GCC conventions of using a clobber with match_scratch. > > > Because the constraint did not include a '&', the temporary register could have > > > used one of the input registers. > > > > > --- gcc/config/rs6000/rs6000.md (revision 261512) > > > +++ gcc/config/rs6000/rs6000.md (working copy) > > > @@ -14102,11 +14102,8 @@ (define_expand "copysign<mode>3" > > > emit_insn (gen_copysign<mode>3_hard (operands[0], operands[1], > > > operands[2])); > > > else > > > - { > > > - rtx tmp = gen_reg_rtx (<MODE>mode); > > > - emit_insn (gen_copysign<mode>3_soft (operands[0], operands[1], > > > - operands[2], tmp)); > > > - } > > > + emit_insn (gen_copysign<mode>3_soft (operands[0], operands[1], > > > + operands[2])); > > > DONE; > > > }) > > > > > > @@ -14125,9 +14122,9 @@ (define_insn "copysign<mode>3_soft" > > > [(set (match_operand:IEEE128 0 "altivec_register_operand" "=v") > > > (unspec:IEEE128 > > > [(match_operand:IEEE128 1 "altivec_register_operand" "v") > > > - (match_operand:IEEE128 2 "altivec_register_operand" "v") > > > - (match_operand:IEEE128 3 "altivec_register_operand" "+v")] > > > - UNSPEC_COPYSIGN))] > > > + (match_operand:IEEE128 2 "altivec_register_operand" "v")] > > > + UNSPEC_COPYSIGN)) > > > + (clobber (match_scratch:IEEE128 3 "=&v"))] > > > "!TARGET_FLOAT128_HW && FLOAT128_IEEE_P (<MODE>mode)" > > > "xscpsgndp %x3,%x2,%x1\;xxpermdi %x0,%x3,%x1,1" > > > [(set_attr "type" "veccomplex") > > > > Does this have to be two machine insns for one RTL insns? If you split it > > in the expander you don't need the clobber or the earlyclobber or anything > > like that. > > > > Okay if splitting it leads to problems elsewhere. > > I suspect it cannot be split before register allocation, just because that has > been a source of problems in the past. Please try. Or, just commit it and I will do the work. Segher
On Fri, Jun 15, 2018 at 08:43:52PM +0000, Joseph Myers wrote: > On Thu, 14 Jun 2018, Michael Meissner wrote: > > > Any other libgcc function that has a long double interface should get the > > linker warning that the wrong type was used. > > libgcc functions have interfaces corresponding to a particular machine > mode and names that vary depending on that mode - not interfaces directly > involving a type such as long double. The interfaces for ibm128 format > are generally *tf* (or __gcc_q*) and those for ieee128 format are > generally *kf*. As far as I know, all the previously observed issues with > some functions getting built for the wrong format have by now been fixed, > and if any more such issues arise in future, they are simply libgcc bugs > to be fixed in libgcc. > > Because the names depend on the format not the type, there is no risk of > ABI inconsistency from linking with a libgcc function for the wrong > format, and so I think the linker warning is never relevant for linking > with libgcc (and once the glibc support is in, it won't be relevant for > linking with glibc either). Linker warnings are relevant for the generic > case of a library that *doesn't* do anything special to support both > formats simultaneously - not for a library that properly handles getting > things right for both formats automatically. Yes. And as far as I see the same is true for *all* libgcc functions (not only long double), so we can just as well turn off the gnu_attributes warning for all of libgcc. Segher
On Tue, 19 Jun 2018, Segher Boessenkool wrote: > > Because the names depend on the format not the type, there is no risk of > > ABI inconsistency from linking with a libgcc function for the wrong > > format, and so I think the linker warning is never relevant for linking > > with libgcc (and once the glibc support is in, it won't be relevant for > > linking with glibc either). Linker warnings are relevant for the generic > > case of a library that *doesn't* do anything special to support both > > formats simultaneously - not for a library that properly handles getting > > things right for both formats automatically. > > Yes. And as far as I see the same is true for *all* libgcc functions > (not only long double), so we can just as well turn off the gnu_attributes > warning for all of libgcc. Yes. libgcc and (future) glibc don't need that warning. (As I noted in <https://gcc.gnu.org/ml/gcc-patches/2018-06/msg01092.html> there *is* currently a bug where some *tf* and *tc* functions get built with the wrong ABI if you change the default long double format.)
Index: libgcc/config/rs6000/t-float128 =================================================================== --- libgcc/config/rs6000/t-float128 (revision 261338) +++ libgcc/config/rs6000/t-float128 (working copy) @@ -59,7 +59,7 @@ fp128_includes = $(srcdir)/soft-fp/doub # Build the emulator without ISA 3.0 hardware support. FP128_CFLAGS_SW = -Wno-type-limits -mvsx -mfloat128 \ - -mno-float128-hardware \ + -mno-float128-hardware -mno-gnu-attribute \ -I$(srcdir)/soft-fp \ -I$(srcdir)/config/rs6000 \ $(FLOAT128_HW_INSNS) Index: libgcc/config/rs6000/t-float128-hw =================================================================== --- libgcc/config/rs6000/t-float128-hw (revision 261338) +++ libgcc/config/rs6000/t-float128-hw (working copy) @@ -25,7 +25,7 @@ fp128_sed_hw = -hw # Build the hardware support functions with appropriate hardware support FP128_CFLAGS_HW = -Wno-type-limits -mvsx -mfloat128 \ -mpower8-vector -mpower9-vector \ - -mfloat128-hardware \ + -mfloat128-hardware -mno-gnu-attribute \ -I$(srcdir)/soft-fp \ -I$(srcdir)/config/rs6000 \ $(FLOAT128_HW_INSNS)