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<mode>"
> + [(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
