[PATCH-4,rs6000] Implement optab_isnormal for SFmode, DFmode and TFmode [PR97786]

Message ID ca60bf46-6ec1-47f1-a5d9-b4fe94db47fe@linux.ibm.com
State New
Headers
Series [PATCH-4,rs6000] Implement optab_isnormal for SFmode, DFmode and TFmode [PR97786] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 fail Patch failed to apply

Commit Message

HAO CHEN GUI April 12, 2024, 8:24 a.m. UTC
  Hi,
  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

  Bootstrapped and tested on powerpc64-linux BE and LE with no
regressions. Is it OK for next stage 1?

Thanks
Gui Haochen


ChangeLog
rs6000: Implement optab_isnormal for SFmode, DFmode and TFmode

gcc/
	PR target/97786
	* config/rs6000/vsx.md (isnormal<mode>2): New expand for SFmode and
	DFmode.
	(isnormal<mode>2): New expand for TFmode.

gcc/testsuite/
	PR target/97786
	* gcc.target/powerpc/pr97786-7.c: New test.
	* gcc.target/powerpc/pr97786-8.c: New test.

patch.diff
  

Comments

Segher Boessenkool May 16, 2024, 5:25 p.m. UTC | #1
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
  
HAO CHEN GUI May 17, 2024, 2:38 a.m. UTC | #2
Hi Segher,
  Thanks for your review comments. I will modify it and resend. Just
one question on the insn condition.

在 2024/5/17 1:25, Segher Boessenkool 写道:
>> +(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?

This expand calls gen_xststdc<sd>p which is a P9 vector instruction and
relies on "TARGET_P9_VECTOR". So I set the condition.
  
Segher Boessenkool May 17, 2024, 1:46 p.m. UTC | #3
On Fri, May 17, 2024 at 10:38:54AM +0800, HAO CHEN GUI wrote:
> This expand calls gen_xststdc<sd>p which is a P9 vector instruction and
> relies on "TARGET_P9_VECTOR". So I set the condition.

Why?  It needs P9, sure, and MSR[VSX] set, but the operands being VSX
registers takes care of that, heh.

But it's fine, the insn patterns it uses use the same conditions
already.


Segher
  

Patch

diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
index a6c72ae33b0..d1c9ef5447c 100644
--- 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"
+{
+  rtx tmp = can_create_pseudo_p () ? gen_reg_rtx (SImode) : operands[0];
+  emit_insn (gen_xststdc<sd>p (tmp, operands[1], GEN_INT (0x7f)));
+  emit_insn (gen_xorsi3 (operands[0], tmp, const1_rtx));
+  DONE;
+})
+
+(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;
+})
+

 ;; The VSX Scalar Test Negative Quad-Precision
 (define_expand "xststdcnegqp_<mode>"
diff --git a/gcc/testsuite/gcc.target/powerpc/pr97786-7.c b/gcc/testsuite/gcc.target/powerpc/pr97786-7.c
new file mode 100644
index 00000000000..a0d848497b9
--- /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" } */
+
+int test1 (double x)
+{
+  return __builtin_isnormal (x);
+}
+
+int test2 (float x)
+{
+  return __builtin_isnormal (x);
+}
+
+/* { dg-final { scan-assembler-not {\mfcmpu\M} } } */
+/* { dg-final { scan-assembler-times {\mxststdc[sd]p\M} 2 } } */
diff --git a/gcc/testsuite/gcc.target/powerpc/pr97786-8.c b/gcc/testsuite/gcc.target/powerpc/pr97786-8.c
new file mode 100644
index 00000000000..d591073d281
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr97786-8.c
@@ -0,0 +1,13 @@ 
+/* { dg-do compile { target lp64 } } */
+/* { 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" } */
+
+int test1 (long double x)
+{
+  return __builtin_isnormal (x);
+}
+
+
+/* { dg-final { scan-assembler-not {\mxscmpuqp\M} } } */
+/* { dg-final { scan-assembler {\mxststdcqp\M} } } */