Re: [PATCH 1/2] Add IEEE 128-bit min/max support on PowerPC.

[will schmidt <will_schmidt@vnet.ibm.com>]

On Tue, 2021-05-18 at 16:26 -0400, Michael Meissner wrote:
> [PATCH 1/2] Add IEEE 128-bit min/max support on PowerPC.
> 

Hi,


> This patch adds the support for the IEEE 128-bit floating point C minimum and
> maximum instructions.  The next patch will add the support for using the
> compare and set mask instruction to implement conditional moves.
> 
> This patch does not try to re-use the code used for SF/DF min/max
> support.  It defines a separate insn for the IEEE 128-bit support.  It
> uses the code iterator <minmax> to simplify adding both operations.
> 
> GCC will not convert ?: operations into using min/max instructions provided in

I'd throw the ternary term in there, easier to search for later. 
s/?: operations/ternary (?:) operations /

> this patch unless the user uses -Ofast or similar switches due to issues with
> NaNs.  The next patch that adds conditional move instructions will enable the
> ?: conversion in many cases.
> 
> I have done bootstrap builds with this patch on the following 3 systems:
>     1)	power9 running LE Linux using --with-cpu=power9
>     2)	power8 running BE Linux using --with-cpu=power8, testing both
> 	32/64-bit.
>     3)	power10 prototype running LE Linux using --with-cpu=power10.
> 
> There were no regressions to the tests, and the new test added passed.  Can I
> check these patches into trunk branch for GCC 12?
> 
> I would like to check these patches into GCC 11 after a cooling off period, but
> I can also not do the backport if desired.
> 
> gcc/
> 2021-05-18  Michael Meissner  <meissner@linux.ibm.com>
> 
> 	* config/rs6000/rs6000.c (rs6000_emit_minmax): Add support for ISA
> 	3.1   IEEE   128-bit   floating  point   xsmaxcqp   and   xsmincqp
> 	instructions.
> 	* config/rs6000/rs6000.md (s<minmax><mode>3, IEEE128 iterator):
> 	New insns.

ok

> 
> gcc/testsuite/
> 2021-05-18  Michael Meissner  <meissner@linux.ibm.com>
> 
> 	* gcc.target/powerpc/float128-minmax-2.c: New test.
> 	* gcc.target/powerpc/float128-minmax.c: Turn off power10 code
> 	generation.

So, presumably the float128-minmax-2.c test adds/replaces the power10
code gen tests that were removed or disabled from float128-minmax.c. 



> ---
>  gcc/config/rs6000/rs6000.c                        |  3 ++-
>  gcc/config/rs6000/rs6000.md                       | 11 +++++++++++
>  .../gcc.target/powerpc/float128-minmax-2.c        | 15 +++++++++++++++
>  .../gcc.target/powerpc/float128-minmax.c          |  7 +++++++
>  4 files changed, 35 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/float128-minmax-2.c
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index 0d0595dddd6..fdaf12aeda0 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -16111,7 +16111,8 @@ rs6000_emit_minmax (rtx dest, enum rtx_code code, rtx op0, rtx op1)
>    /* VSX/altivec have direct min/max insns.  */
>    if ((code == SMAX || code == SMIN)
>        && (VECTOR_UNIT_ALTIVEC_OR_VSX_P (mode)
> -	  || (mode == SFmode && VECTOR_UNIT_VSX_P (DFmode))))
> +	  || (mode == SFmode && VECTOR_UNIT_VSX_P (DFmode))
> +	  || (TARGET_POWER10 && TARGET_FLOAT128_HW && FLOAT128_IEEE_P (mode))))
>      {
>        emit_insn (gen_rtx_SET (dest, gen_rtx_fmt_ee (code, mode, op0, op1)));
>        return;
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 0bfeb24d9e8..3a1bc1f8547 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -5196,6 +5196,17 @@ (define_insn "*s<minmax><mode>3_vsx"
>  }
>    [(set_attr "type" "fp")])
> 
> +;; Min/max for ISA 3.1 IEEE 128-bit floating point
> +(define_insn "s<minmax><mode>3"
> +  [(set (match_operand:IEEE128 0 "altivec_register_operand" "=v")
> +	(fp_minmax:IEEE128
> +	 (match_operand:IEEE128 1 "altivec_register_operand" "v")
> +	 (match_operand:IEEE128 2 "altivec_register_operand" "v")))]
> +  "TARGET_POWER10"
> +  "xs<minmax>cqp %0,%1,%2"
> +  [(set_attr "type" "vecfloat")
> +   (set_attr "size" "128")])
> +
>  ;; The conditional move instructions allow us to perform max and min operations
>  ;; even when we don't have the appropriate max/min instruction using the FSEL
>  ;; instruction.

ok


> diff --git a/gcc/testsuite/gcc.target/powerpc/float128-minmax-2.c b/gcc/testsuite/gcc.target/powerpc/float128-minmax-2.c
> new file mode 100644
> index 00000000000..c71ba08c9f8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/float128-minmax-2.c
> @@ -0,0 +1,15 @@
> +/* { dg-require-effective-target ppc_float128_hw } */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-options "-mdejagnu-cpu=power10 -O2 -ffast-math" } */
> +
> +#ifndef TYPE
> +#define TYPE _Float128
> +#endif
> +
> +/* Test that the fminf128/fmaxf128 functions generate if/then/else and not a
> +   call.  */
> +TYPE f128_min (TYPE a, TYPE b) { return __builtin_fminf128 (a, b); }
> +TYPE f128_max (TYPE a, TYPE b) { return __builtin_fmaxf128 (a, b); }
> +
> +/* { dg-final { scan-assembler {\mxsmaxcqp\M} } } */
> +/* { dg-final { scan-assembler {\mxsmincqp\M} } } */

ok

> diff --git a/gcc/testsuite/gcc.target/powerpc/float128-minmax.c b/gcc/testsuite/gcc.target/powerpc/float128-minmax.c
> index fe397518f2f..c3af759c0b9 100644
> --- a/gcc/testsuite/gcc.target/powerpc/float128-minmax.c
> +++ b/gcc/testsuite/gcc.target/powerpc/float128-minmax.c
> @@ -3,6 +3,13 @@
>  /* { dg-require-effective-target float128 } */
>  /* { dg-options "-mpower9-vector -O2 -ffast-math" } */
> 
> +/* If the compiler was configured to automatically generate power10 support with
> +   --with-cpu=power10, turn it off.  Otherwise, it will generate XXMAXCQP and
> +   XXMINCQP instructions.  */
> +#ifdef _ARCH_PWR10
> +#pragma GCC target ("cpu=power9")
> +#endif

Probably fine..  It's good to exercise the pragma target stuff, thoguh
I wonder if it would be better to just specify -mcpu=power9 in the
options since we are already specifying (redundant?) -mpower9-vector. 

I see similar changes in a later patch, probably OK there since those
tests do not appear to be specifying -mcpu=foo options that are already
pointed at power9 features...


> +
>  #ifndef TYPE
>  #define TYPE _Float128
>  #endif
> -- 
> 2.31.1

lgtm, 
thanks
-Will

> 
>