, PowerPC long double transistion, patch #1

Message ID 20180613211015.GA3737@ibm-toto.the-meissners.org
State New
Headers show
Series
  • , PowerPC long double transistion, patch #1
Related show

Commit Message

Michael Meissner June 13, 2018, 9:10 p.m.
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.

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.

Comments

Michael Meissner June 13, 2018, 9:21 p.m. | #1
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.
Michael Meissner June 13, 2018, 9:25 p.m. | #2
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.
Michael Meissner June 13, 2018, 9:45 p.m. | #3
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.
Segher Boessenkool June 14, 2018, 11:25 p.m. | #4
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
Michael Meissner June 15, 2018, 12:18 a.m. | #5
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.
Segher Boessenkool June 15, 2018, 6:39 p.m. | #6
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
Michael Meissner June 15, 2018, 7:56 p.m. | #7
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.
Joseph Myers June 15, 2018, 8:43 p.m. | #8
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.
Segher Boessenkool June 15, 2018, 9:23 p.m. | #9
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
Segher Boessenkool June 19, 2018, 5:24 p.m. | #10
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
Segher Boessenkool June 19, 2018, 8:58 p.m. | #11
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
Joseph Myers June 19, 2018, 11:05 p.m. | #12
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.)

Patch

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)