[PATCH 2/2] PowerPC: Add power10 IEEE 128-bit min/max/cmove.

[Segher Boessenkool]

Hi!

On Tue, Aug 11, 2020 at 12:23:07PM -0400, Michael Meissner wrote:
> +  /* See if we can use the ISA 3.1 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 is that?  That makes the code quite different too (harder to
review), but also, it could just use existing code more.

> +;; IEEE 128-bit min/max
> +(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_FLOAT128_HW && TARGET_POWER10 && FLOAT128_IEEE_P (<MODE>mode)"
> +  "xs<minmax>cqp %0,%1,%2"
> +  [(set_attr "type" "fp")
> +   (set_attr "size" "128")])

So why do we want the asymmetrical xsmincqp instead of xsminqp?  This
should be documented at the very least.  (We also should have min/max
that work correctly without -ffast-math, of course :-( ).

> +;; IEEE 128-bit conditional move.  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.

(Unmatched brackets).  So why is this?  "At present" doesn't belong in
the code btw, but in your patch description.

> +(define_insn_and_split "*mov<mode>cc_hardware"

Please don't use meaningless names like this.

> +(define_insn "*xxsel<mode>"

This already exists for other vector modes.  Put it together with that
please.  Same for all other insn patterns.

Ideally you can make those one pattern then, even.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/float128-minmax-2.c
> @@ -0,0 +1,70 @@
> +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-options "-mdejagnu-cpu=power10 -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"     } } */

Use \m \M please.  Don't ask for powerpc* in gcc.target/powerpc/ (it is
implied there).  Why does it need lp64?  There should be some test for
__float128, instead.

All in all, this is very hard to review :-(


Segher