Hi!
On Fri, Apr 12, 2024 at 04:24:23PM +0800, HAO CHEN GUI wrote:
> This patch implemented optab_isnormal for SF/DF/TFmode by rs6000 test
> data class instructions.
>
> This patch relies on former patch which adds optab_isnormal.
> https://gcc.gnu.org/pipermail/gcc-patches/2024-April/649366.html
> gcc/
> PR target/97786
> * config/rs6000/vsx.md (isnormal<mode>2): New expand for SFmode and
> DFmode.
* config/rs6000/vsx.md (isnormal<mode>2 for SFDF): New expand.
(isnormal<mode>2 for IEEE128): New expand.
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -5357,6 +5357,30 @@ (define_expand "isfinite<mode>2"
> DONE;
> })
>
> +(define_expand "isnormal<mode>2"
> + [(use (match_operand:SI 0 "gpc_reg_operand"))
> + (use (match_operand:SFDF 1 "gpc_reg_operand"))]
> + "TARGET_HARD_FLOAT
> + && TARGET_P9_VECTOR"
Please put the condition on just one line if it is as simple and short
as this.
Why is TARGET_P9_VECTOR the correct condition?
> +{
> + rtx tmp = can_create_pseudo_p () ? gen_reg_rtx (SImode) : operands[0];
This is an expander. can_create_pseudo_p always return true. Please
simplify the code, keeping that in mind :-)
> +(define_expand "isnormal<mode>2"
> + [(use (match_operand:SI 0 "gpc_reg_operand"))
> + (use (match_operand:IEEE128 1 "gpc_reg_operand"))]
> + "TARGET_HARD_FLOAT
> + && TARGET_P9_VECTOR"
> +{
> + rtx tmp = can_create_pseudo_p () ? gen_reg_rtx (SImode) : operands[0];
> + emit_insn (gen_xststdcqp_<mode> (tmp, operands[1], GEN_INT (0x7f)));
> + emit_insn (gen_xorsi3 (operands[0], tmp, const1_rtx));
> + DONE;
> +})
Same issues here, of course.
> +
Why add radom white lines? Pleaase don't.
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr97786-7.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target powerpc_vsx_ok } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power9 -mvsx" } */
If you use a -mcpu=, don't use vsx_ok.
If you use a -mcpu=, don't use -mvsx.
> +int test1 (double x)
> +{
> + return __builtin_isnormal (x);
> +}
> +
> +int test2 (float x)
> +{
> + return __builtin_isnormal (x);
> +}
> +
> +/* { dg-final { scan-assembler-not {\mfcmpu\M} } } */
Just \mfcmp please (so that it also catches fcmpo, if we ever generate
that).
> +/* { dg-final { scan-assembler-times {\mxststdc[sd]p\M} 2 } } */
Maybe you should test for one each of the s and d version? So just
/* { dg-final { scan-assembler-times {\mxststdcsp\M} 1 } } */
/* { dg-final { scan-assembler-times {\mxststdcdp\M} 1 } } */
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr97786-8.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile { target lp64 } } */
Why run this on 64-bit systems only? If there is a reason, document
that here (but is there a reason?)
> +/* { dg-require-effective-target ppc_float128_sw } */
> +/* { dg-require-effective-target powerpc_vsx_ok } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power9 -mvsx -mabi=ieeelongdouble
> -Wno-psabi" } */
Same comments here: If you have a -mcpu you do not want vsx_ok or -mvsx.
Please fix these things and resend. Thanks!
Segher
