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
