Re: [PATCH 3/3] PowerPC future: Add IEEE 128-bit min, max, compare.
Hi!
Everything Will said included by reference :-)
On Mon, Jun 01, 2020 at 04:01:53PM -0400, Michael Meissner wrote:
> Add support for the new IEEE 128-bit minimum, maximum, and set compare mask
> instructions when -mcpu=future was used.
You can say "ISA 3.1 instructions" now :-) That may read better in the
commit message? OTOH, we'll keep using the "future" name for this in
the actual code a little longer, until everything is in, to keep things
easier for everyone (and then change everything at once).
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -14847,7 +14847,9 @@ rs6000_emit_vector_cond_expr (rtx dest, rtx op_true,
> rtx op_false,
> /* ISA 3.0 (power9) minmax subcase to emit a XSMAXCDP or XSMINCDP instruction
> for SF/DF scalars. Move TRUE_COND to DEST if OP of the operands of the
> last
> comparison is nonzero/true, FALSE_COND if it is zero/false. Return 0 if
> the
> - hardware has no such operation. */
> + hardware has no such operation.
> +
> + Under FUTURE, also handle IEEE 128-bit floating point. */
Don't say this please: if you do, eventually we'll end up with a huge
list of how every function does its work for every ISA version. Instead,
describe it at a higher level, if you want to say anything at all?
Replace the "SF/DF" with "supported scalar floating point modes" or such.
> @@ -14981,6 +14985,21 @@ rs6000_emit_cmove (rtx dest, rtx op, rtx true_cond,
> rtx false_cond)
> return 1;
> }
>
> + /* See if we can use the FUTURE min/max/compare instructions for IEEE
> 128-bit
> + floating point. At present, don't worry about doing conditional moves
> + with different types for the comparison and movement (unlike SF/DF,
> where
> + you can do a conditional test between double and use float as the
> if/then
> + parts. */
> + if (TARGET_FUTURE && FLOAT128_IEEE_P (compare_mode)
> + && compare_mode == result_mode)
> +{
> + if (rs6000_emit_hw_fp_minmax (dest, op, true_cond, false_cond))
> + return 1;
> +
> + if (rs6000_emit_hw_fp_cmove (dest, op, true_cond, false_cond))
> + return 1;
> +}
"emit_hw_minmax" sounds completely wrong to implement a conditional move.
Presumably the name of that function isn't really what it does? It would
make a lot more sense already if there was "try" in the name. A function
called "emit" should not return a bool saying if it did the job -- if it
didn't it should ICE!
> +(define_insn "*xxsel"
> + [(set (match_operand:IEEE128 0 "vsx_register_operand" "=wa")
> + (if_then_else:IEEE128
> + (ne (match_operand:V2DI 1 "vsx_register_operand" "wa")
> + (match_operand:V2DI 2 "zero_constant" ""))
Leave out the constraint string completely?
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/float128-minmax-2.c
> @@ -0,0 +1,70 @@
> +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
Everything in this dir is always only used for powerpc targets; don't
test for that again per testcase please.
> +/* { dg-require-effective-target powerpc_future_ok } */
> +/* { dg-options "-mdejagnu-cpu=future -O2 -ffast-math" } */
> +/* { dg-final { scan-assembler-not "xscmpuqp" } } */
> +/* { dg-final { scan-assembler "xscmpeqqp" } } */
> +/* { dg-final { scan-assembler "xscmpgtqp" } } */
> +/* { dg-final { scan-assembler "xscmpgeqp" } } */
> +/* { dg-final { scan-assembler "xsmaxcqp" } } */
> +/* { dg-final { scan-assembler "xsmincqp" } } */
> +/* { dg-final { scan-assembler "xxsel" } } */
\m and \M please. And could you use scan-assembler-times here? If so,
please do (all of this is very general advice).
Segher
Re: [PATCH 3/3] PowerPC future: Add IEEE 128-bit min, max, compare.
On Mon, 2020-06-01 at 16:01 -0400, Michael Meissner via Gcc-patches wrote:
> Add support for the new IEEE 128-bit minimum, maximum, and set compare mask
> instructions when -mcpu=future was used.
>
> gcc/
> 2020-06-01 Michael Meissner
>
> * config/rs6000/rs6000.c (rs6000_emit_hw_fp_minmax): Update
> comment.
> (rs6000_emit_hw_fp_cmove): Update comment.
> (rs6000_emit_cmove): Add support for IEEE 128-bit min, max, and
> comparisons with -mcpu=future.
> (rs6000_emit_minmax): Add support for IEEE 128-bit min/max with
> -mcpu=future.
> * config/rs6000/rs6000.md (s3, IEEE128 iterator):
> New insns for IEEE 128-bit min/max.
> (movcc, IEEE128 iterator): New insns for IEEE 128-bit
> conditional move.
> (movcc_future, IEEE128 iterator): New insns for IEEE 128-bit
> conditional move.
> (movcc_invert_future, IEEE128 iterator): New insns for IEEE
> 128-bit conditional move.
> (fpmask, IEEE128 iterator): New insns for IEEE 128-bit
> conditional move.
Include the leading wildcard here?
(*fpmask ...
and missing an entry for this one:
(*xxsel ...
>
> testsuite/
> 2020-06-01 Michael Meissner
>
> * gcc.target/powerpc/float128-minmax-2.c: New test.
> ---
> gcc/config/rs6000/rs6000.c | 26 -
> gcc/config/rs6000/rs6000.md| 121
> +
> .../gcc.target/powerpc/float128-minmax-2.c | 70
> 3 files changed, 214 insertions(+), 3 deletions(-)
> 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 0921328..bbba8f1 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -14847,7 +14847,9 @@ rs6000_emit_vector_cond_expr (rtx dest, rtx op_true,
> rtx op_false,
> /* ISA 3.0 (power9) minmax subcase to emit a XSMAXCDP or XSMINCDP instruction
> for SF/DF scalars. Move TRUE_COND to DEST if OP of the operands of the
> last
> comparison is nonzero/true, FALSE_COND if it is zero/false. Return 0 if
> the
> - hardware has no such operation. */
> + hardware has no such operation.
> +
> + Under FUTURE, also handle IEEE 128-bit floating point. */
>
> static int
> rs6000_emit_hw_fp_minmax (rtx dest, rtx op, rtx true_cond, rtx false_cond)
> @@ -14889,7 +14891,9 @@ rs6000_emit_hw_fp_minmax (rtx dest, rtx op, rtx
> true_cond, rtx false_cond)
> /* ISA 3.0 (power9) conditional move subcase to emit XSCMP{EQ,GE,GT,NE}DP and
> XXSEL instructions for SF/DF scalars. Move TRUE_COND to DEST if OP of the
> operands of the last comparison is nonzero/true, FALSE_COND if it is
> - zero/false. Return 0 if the hardware has no such operation. */
> + zero/false. Return 0 if the hardware has no such operation.
> +
> + Under FUTURE, also handle IEEE 128-bit conditional moves. */
>
> static int
> rs6000_emit_hw_fp_cmove (rtx dest, rtx op, rtx true_cond, rtx false_cond)
> @@ -14981,6 +14985,21 @@ rs6000_emit_cmove (rtx dest, rtx op, rtx true_cond,
> rtx false_cond)
> return 1;
> }
>
> + /* See if we can use the FUTURE min/max/compare instructions for IEEE
> 128-bit
> + floating point. At present, don't worry about doing conditional moves
> + with different types for the comparison and movement (unlike SF/DF,
> where
> + you can do a conditional test between double and use float as the
> if/then
> + parts. */
Why don't we worry about that now? Should this be a 'future todo'
comment here?
Beyond those nits and questions, lgtm,
Thanks,
-Will
