Aarch64: Add missing clobber for fjcvtzs

Message ID gkr36582kyl.fsf@arm.com
State New
Headers show
Series
  • Aarch64: Add missing clobber for fjcvtzs
Related show

Commit Message

Andrea Corallo July 31, 2020, 7:38 a.m.
Hi all,

I'd like to submit the following patch that adds a missing CC reg
clobber for the FJCVTZS instruction [1].

The patch adds an executing testcase that fails without the clobber.

Bootstrapped on aarch64-linux-gnu, regression ongoing.

Okay for trunk when finished testing?

  Andrea

[1] https://developer.arm.com/docs/ddi0596/latest/simd-and-floating-point-instructions-alphabetic-order/fjcvtzs-floating-point-javascript-convert-to-signed-fixed-point-rounding-toward-zero

gcc/ChangeLog

2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

	* config/aarch64/aarch64.md (aarch64_fjcvtzs): Add missing
	clobber.

gcc/testsuite/ChangeLog

2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

	* gcc.target/aarch64/acle/jcvt_2.c: New testcase.
	* lib/target-supports.exp
	(check_effective_target_aarch64_fjcvtzs_hw): Add new check for
	FJCVTZS hw.
From 419436f9717a18545e5e89dff7f28e6958891f4c Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>

Date: Wed, 29 Jul 2020 19:04:40 +0200
Subject: [PATCH] Add missing clobber for fjcvtzs

gcc/ChangeLog

2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

	* config/aarch64/aarch64.md (aarch64_fjcvtzs): Add missing
	clobber.

gcc/testsuite/ChangeLog

2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

	* gcc.target/aarch64/acle/jcvt_2.c: New testcase.
	* lib/target-supports.exp
	(check_effective_target_aarch64_fjcvtzs_hw): Add new check for
	FJCVTZS hw.
---
 gcc/config/aarch64/aarch64.md                 |  3 +-
 .../gcc.target/aarch64/acle/jcvt_2.c          | 33 +++++++++++++++++++
 gcc/testsuite/lib/target-supports.exp         | 21 ++++++++++++
 3 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/acle/jcvt_2.c

-- 
2.17.1

Comments

Richard Sandiford July 31, 2020, 9:39 a.m. | #1
Andrea Corallo <andrea.corallo@arm.com> writes:
> Hi all,

>

> I'd like to submit the following patch that adds a missing CC reg

> clobber for the FJCVTZS instruction [1].

>

> The patch adds an executing testcase that fails without the clobber.

>

> Bootstrapped on aarch64-linux-gnu, regression ongoing.

>

> Okay for trunk when finished testing?

>

>   Andrea

>

> [1] https://developer.arm.com/docs/ddi0596/latest/simd-and-floating-point-instructions-alphabetic-order/fjcvtzs-floating-point-javascript-convert-to-signed-fixed-point-rounding-toward-zero

>

> gcc/ChangeLog

>

> 2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

>

> 	* config/aarch64/aarch64.md (aarch64_fjcvtzs): Add missing

> 	clobber.

>

> gcc/testsuite/ChangeLog

>

> 2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

>

> 	* gcc.target/aarch64/acle/jcvt_2.c: New testcase.

> 	* lib/target-supports.exp

> 	(check_effective_target_aarch64_fjcvtzs_hw): Add new check for

> 	FJCVTZS hw.


OK, thanks.  Also OK for any branches that need it.

As a follow-up, it might be nice at some point to have a pattern that
takes advantage of the Z flag result, but that's obviously much less
important than fixing the bug.

Richard
Andrea Corallo Aug. 4, 2020, 8:27 a.m. | #2
Hi all,

looking into the regression I realized I've got the condition into the
effective target test wrong (== vs !=).  Apologies for the noise.

Bootstrapped on aarch64-none-linux-gnu and regresssioned on
aarch64-none-elf.

Still okay for trunk and to backport on gcc-10?

Thanks

  Andrea

gcc/ChangeLog

2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

	* config/aarch64/aarch64.md (aarch64_fjcvtzs): Add missing
	clobber.

gcc/testsuite/ChangeLog

2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

	* gcc.target/aarch64/acle/jcvt_2.c: New testcase.
	* lib/target-supports.exp
	(check_effective_target_aarch64_fjcvtzs_hw): Add new check for
	FJCVTZS hw.
From 573b7a09f8c87621efeef80ea42055630391a40c Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>

Date: Wed, 29 Jul 2020 19:04:40 +0200
Subject: [PATCH] Add missing clobber for fjcvtzs

gcc/ChangeLog

2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

	* config/aarch64/aarch64.md (aarch64_fjcvtzs): Add missing
	clobber.

gcc/testsuite/ChangeLog

2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

	* gcc.target/aarch64/acle/jcvt_2.c: New testcase.
	* lib/target-supports.exp
	(check_effective_target_aarch64_fjcvtzs_hw): Add new check for
	FJCVTZS hw.
---
 gcc/config/aarch64/aarch64.md                 |  3 +-
 .../gcc.target/aarch64/acle/jcvt_2.c          | 33 +++++++++++++++++++
 gcc/testsuite/lib/target-supports.exp         | 21 ++++++++++++
 3 files changed, 56 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/acle/jcvt_2.c

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index d5ca1898c02e..df780b863707 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -7059,7 +7059,8 @@
 (define_insn "aarch64_fjcvtzs"
   [(set (match_operand:SI 0 "register_operand" "=r")
 	(unspec:SI [(match_operand:DF 1 "register_operand" "w")]
-		   UNSPEC_FJCVTZS))]
+		   UNSPEC_FJCVTZS))
+   (clobber (reg:CC CC_REGNUM))]
   "TARGET_JSCVT"
   "fjcvtzs\\t%w0, %d1"
   [(set_attr "type" "f_cvtf2i")]
diff --git a/gcc/testsuite/gcc.target/aarch64/acle/jcvt_2.c b/gcc/testsuite/gcc.target/aarch64/acle/jcvt_2.c
new file mode 100644
index 000000000000..ea2dfd14cf29
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/acle/jcvt_2.c
@@ -0,0 +1,33 @@
+/* Test the __jcvt ACLE intrinsic.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -march=armv8.3-a -save-temps" } */
+/* { dg-require-effective-target aarch64_fjcvtzs_hw } */
+
+#include <arm_acle.h>
+
+extern void abort (void);
+
+#ifdef __ARM_FEATURE_JCVT
+volatile int32_t x;
+
+int __attribute__((noinline))
+foo (double a, int b, int c)
+{
+  b = b > c;
+  x = __jcvt (a);
+  return b;
+}
+
+int
+main (void)
+{
+  int x = foo (1.1, 2, 3);
+  if (x)
+    abort ();
+
+  return 0;
+}
+
+#endif
+
+/* { dg-final { scan-assembler-times "fjcvtzs\tw\[0-9\]+, d\[0-9\]+\n" 1 } } */
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 57eed3012b94..885689675e1d 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -4848,6 +4848,27 @@ proc check_effective_target_aarch64_bti_hw { } {
     } "-O2" ]
 }
 
+# Return 1 if the target supports executing the armv8.3-a FJCVTZS
+# instruction.
+proc check_effective_target_aarch64_fjcvtzs_hw { } {
+    if { ![istarget aarch64*-*-*] } {
+	return 0
+    }
+    return [check_runtime aarch64_fjcvtzs_hw_available {
+	int
+	main (void)
+	{
+	  double in = 25.1;
+	  int out;
+	  asm volatile ("fjcvtzs %w0, %d1"
+			: "=r" (out)
+			: "w" (in)
+			: /* No clobbers.  */);
+	  return out != 25;
+	}
+    } "-march=armv8.3-a" ]
+}
+
 # Return 1 if GCC was configured with --enable-standard-branch-protection
 proc check_effective_target_default_branch_protection { } {
     return [check_configured_with "enable-standard-branch-protection"]
-- 
2.17.1
Kyrylo Tkachov Aug. 4, 2020, 8:30 a.m. | #3
Hi Andrea,

> -----Original Message-----

> From: Andrea Corallo <Andrea.Corallo@arm.com>

> Sent: 04 August 2020 09:28

> To: gcc-patches@gcc.gnu.org

> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>; Richard Earnshaw

> <Richard.Earnshaw@arm.com>; Richard Sandiford

> <Richard.Sandiford@arm.com>

> Subject: Re: [PATCH] Aarch64: Add missing clobber for fjcvtzs

> 

> Hi all,

> 

> looking into the regression I realized I've got the condition into the

> effective target test wrong (== vs !=).  Apologies for the noise.

> 

> Bootstrapped on aarch64-none-linux-gnu and regresssioned on

> aarch64-none-elf.

> 

> Still okay for trunk and to backport on gcc-10?


I just remembered a recurring bit of review feedback from Ramana on my patches...
New effective target checks need to be documented in doc/sourcebuild.texi.

Ok with the documentation.
Thanks,
Kyrill

> 

> Thanks

> 

>   Andrea

> 

> gcc/ChangeLog

> 

> 2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

> 

> 	* config/aarch64/aarch64.md (aarch64_fjcvtzs): Add missing

> 	clobber.

> 

> gcc/testsuite/ChangeLog

> 

> 2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

> 

> 	* gcc.target/aarch64/acle/jcvt_2.c: New testcase.

> 	* lib/target-supports.exp

> 	(check_effective_target_aarch64_fjcvtzs_hw): Add new check for

> 	FJCVTZS hw.

>
Andrea Corallo Aug. 4, 2020, 10:26 a.m. | #4
Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> writes:

> I just remembered a recurring bit of review feedback from Ramana on my patches...

> New effective target checks need to be documented in doc/sourcebuild.texi.

>

> Ok with the documentation.

> Thanks,

> Kyrill


Hi Kyrill,

thanks for catching that.

The attached is committed into master as d2b86e14c14.

  Andrea
From d2b86e14c14020f3e119ab8f462e2a91bd7d46e5 Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>

Date: Wed, 29 Jul 2020 19:04:40 +0200
Subject: [PATCH] aarch64: Add missing clobber for fjcvtzs

gcc/ChangeLog

2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

	* config/aarch64/aarch64.md (aarch64_fjcvtzs): Add missing
	clobber.
	* doc/sourcebuild.texi (aarch64_fjcvtzs_hw) Document new
	target supports option.

gcc/testsuite/ChangeLog

2020-07-30  Andrea Corallo  <andrea.corallo@arm.com>

	* gcc.target/aarch64/acle/jcvt_2.c: New testcase.
	* lib/target-supports.exp
	(check_effective_target_aarch64_fjcvtzs_hw): Add new check for
	FJCVTZS hw.
---
 gcc/config/aarch64/aarch64.md                 |  3 +-
 gcc/doc/sourcebuild.texi                      |  3 ++
 .../gcc.target/aarch64/acle/jcvt_2.c          | 33 +++++++++++++++++++
 gcc/testsuite/lib/target-supports.exp         | 21 ++++++++++++
 4 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/acle/jcvt_2.c

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index d5ca1898c02..df780b86370 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -7059,7 +7059,8 @@
 (define_insn "aarch64_fjcvtzs"
   [(set (match_operand:SI 0 "register_operand" "=r")
 	(unspec:SI [(match_operand:DF 1 "register_operand" "w")]
-		   UNSPEC_FJCVTZS))]
+		   UNSPEC_FJCVTZS))
+   (clobber (reg:CC CC_REGNUM))]
   "TARGET_JSCVT"
   "fjcvtzs\\t%w0, %d1"
   [(set_attr "type" "f_cvtf2i")]
diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
index a7a922d84a2..63216a0daba 100644
--- a/gcc/doc/sourcebuild.texi
+++ b/gcc/doc/sourcebuild.texi
@@ -2063,6 +2063,9 @@ whether it does so by default).
 @itemx aarch64_sve2048_hw
 Like @code{aarch64_sve_hw}, but also test for an exact hardware vector length.
 
+@item aarch64_fjcvtzs_hw
+AArch64 target that is able to generate and execute armv8.3-a FJCVTZS
+instruction.
 @end table
 
 @subsubsection MIPS-specific attributes
diff --git a/gcc/testsuite/gcc.target/aarch64/acle/jcvt_2.c b/gcc/testsuite/gcc.target/aarch64/acle/jcvt_2.c
new file mode 100644
index 00000000000..ea2dfd14cf2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/acle/jcvt_2.c
@@ -0,0 +1,33 @@
+/* Test the __jcvt ACLE intrinsic.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -march=armv8.3-a -save-temps" } */
+/* { dg-require-effective-target aarch64_fjcvtzs_hw } */
+
+#include <arm_acle.h>
+
+extern void abort (void);
+
+#ifdef __ARM_FEATURE_JCVT
+volatile int32_t x;
+
+int __attribute__((noinline))
+foo (double a, int b, int c)
+{
+  b = b > c;
+  x = __jcvt (a);
+  return b;
+}
+
+int
+main (void)
+{
+  int x = foo (1.1, 2, 3);
+  if (x)
+    abort ();
+
+  return 0;
+}
+
+#endif
+
+/* { dg-final { scan-assembler-times "fjcvtzs\tw\[0-9\]+, d\[0-9\]+\n" 1 } } */
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index ba9db0be2f9..e79015b4d54 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -4848,6 +4848,27 @@ proc check_effective_target_aarch64_bti_hw { } {
     } "-O2" ]
 }
 
+# Return 1 if the target supports executing the armv8.3-a FJCVTZS
+# instruction.
+proc check_effective_target_aarch64_fjcvtzs_hw { } {
+    if { ![istarget aarch64*-*-*] } {
+	return 0
+    }
+    return [check_runtime aarch64_fjcvtzs_hw_available {
+	int
+	main (void)
+	{
+	  double in = 25.1;
+	  int out;
+	  asm volatile ("fjcvtzs %w0, %d1"
+			: "=r" (out)
+			: "w" (in)
+			: /* No clobbers.  */);
+	  return out != 25;
+	}
+    } "-march=armv8.3-a" ]
+}
+
 # Return 1 if GCC was configured with --enable-standard-branch-protection
 proc check_effective_target_default_branch_protection { } {
     return [check_configured_with "enable-standard-branch-protection"]
-- 
2.17.1
Andrea Corallo Aug. 4, 2020, 12:48 p.m. | #5
Andrea Corallo <andrea.corallo@arm.com> writes:

> Hi Kyrill,

>

> thanks for catching that.

>

> The attached is committed into master as d2b86e14c14.

>

>   Andrea


And backported in releases/gcc-10 as e5907f3b631.

  Andrea

Patch

diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index d5ca1898c02e..df780b863707 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -7059,7 +7059,8 @@ 
 (define_insn "aarch64_fjcvtzs"
   [(set (match_operand:SI 0 "register_operand" "=r")
 	(unspec:SI [(match_operand:DF 1 "register_operand" "w")]
-		   UNSPEC_FJCVTZS))]
+		   UNSPEC_FJCVTZS))
+   (clobber (reg:CC CC_REGNUM))]
   "TARGET_JSCVT"
   "fjcvtzs\\t%w0, %d1"
   [(set_attr "type" "f_cvtf2i")]
diff --git a/gcc/testsuite/gcc.target/aarch64/acle/jcvt_2.c b/gcc/testsuite/gcc.target/aarch64/acle/jcvt_2.c
new file mode 100644
index 000000000000..ea2dfd14cf29
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/acle/jcvt_2.c
@@ -0,0 +1,33 @@ 
+/* Test the __jcvt ACLE intrinsic.  */
+/* { dg-do run } */
+/* { dg-options "-O2 -march=armv8.3-a -save-temps" } */
+/* { dg-require-effective-target aarch64_fjcvtzs_hw } */
+
+#include <arm_acle.h>
+
+extern void abort (void);
+
+#ifdef __ARM_FEATURE_JCVT
+volatile int32_t x;
+
+int __attribute__((noinline))
+foo (double a, int b, int c)
+{
+  b = b > c;
+  x = __jcvt (a);
+  return b;
+}
+
+int
+main (void)
+{
+  int x = foo (1.1, 2, 3);
+  if (x)
+    abort ();
+
+  return 0;
+}
+
+#endif
+
+/* { dg-final { scan-assembler-times "fjcvtzs\tw\[0-9\]+, d\[0-9\]+\n" 1 } } */
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 57eed3012b94..5973f7edb4b3 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -4848,6 +4848,27 @@  proc check_effective_target_aarch64_bti_hw { } {
     } "-O2" ]
 }
 
+# Return 1 if the target supports executing the armv8.3-a FJCVTZS
+# instruction.
+proc check_effective_target_aarch64_fjcvtzs_hw { } {
+    if { ![istarget aarch64*-*-*] } {
+	return 0
+    }
+    return [check_runtime aarch64_fjcvtzs_hw_available {
+	int
+	main (void)
+	{
+	  double in = 25.1;
+	  int out;
+	  asm volatile ("fjcvtzs %w0, %d1"
+			: "=r" (out)
+			: "w" (in)
+			: /* No clobbers.  */);
+	  return out == 25;
+	}
+    } "-march=armv8.3-a" ]
+}
+
 # Return 1 if GCC was configured with --enable-standard-branch-protection
 proc check_effective_target_default_branch_protection { } {
     return [check_configured_with "enable-standard-branch-protection"]