Re: [PATCH], Add support for __builtin_{sqrt,fma}f128 on PowerPC ISA 3.0

[Segher Boessenkool <segher@kernel.crashing.org>]

On Wed, Sep 13, 2017 at 05:46:00PM -0400, Michael Meissner wrote:
> This patch adds support on PowerPC ISA 3.0 for the built-in function
> __builtin_sqrtf128 generating the XSSQRTQP hardware square root instruction and
> the built-in function __builtin_fmaf128 generating XSMADDQP, XSMSUBQP,
> XSNMADDQP, and XSNMSUBQP fused multiply-add instructions.
> 
> While I was at it, I changed the documentation so that it no longer documents
> the 'q' built-in functions (to mirror libquadmath) but instead just documented
> the 'f128' functions that matches glibc 2.26 and the technical report that
> added the _FloatF128 date.
> 
> I changed the tests that used __fabsq to use __fabsf128 instead.
> 
> I also added && lp64 to float128-5.c so that it doesn't cause errors when doing
> the test for a 32-bit target.  This is due to the fact that if you enable
> hardware IEEE 128-bit floating point, you eventually will need TImode
> supported, and that is not supported on 32-bit targets.
> 
> I did a bootstrap and make check with subversion id 252033 on a little endian
> power8 system.  The subversion id 252033 is one of the last svn ids that
> bootstrap without additional patches on the PowerPC.  There were no regressions
> in this patch, and I verified the 4 new tests were run.  Can I check this patch
> into the trunk?

Yes please.  A few trivial things:

> 	* doc/extend.texi (RS/6000 built-in functions): Document the
> 	'f128' IEEE 128-bit floating point built-in functions.  Don't
> 	document the older 'q' versions of the functions. Document the
> 	built-in IEEE 128-bit floating point square root and fused
> 	multiply-add built-ins.

Dot space space.

> +/* 1 argument IEEE 128-bit floating point functions that require ISA 3.0
> +   hardware.  We define both a 'q' version for libquadmath compatibility, and a
> +   'f128' for glibc 2.26.  We didn't need this for FABS/COPYSIGN, since the
> +   machine independent built-in support already defines the F128 versions,  */

Dot instead of comma?

> --- gcc/testsuite/gcc.target/powerpc/float128-5.c	(revision 252730)
> +++ gcc/testsuite/gcc.target/powerpc/float128-5.c	(working copy)
> @@ -1,4 +1,4 @@
> -/* { dg-do compile { target { powerpc*-*-linux* } } } */
> +/* { dg-do compile { target { powerpc*-*-linux* && lp64 } } } */

Maybe add a comment why this is -m64 only?

Thanks,


Segher