diff mbox series

aarch64: Fix bug with max/min (PR116934)

Message ID 20241004090920.1453806-1-saurabh.jha@arm.com
State New
Headers show
Series aarch64: Fix bug with max/min (PR116934) | expand

Commit Message

Saurabh Jha Oct. 4, 2024, 9:09 a.m. UTC 
In ac4cdf5cb43c0b09e81760e2a1902ceebcf1a135, I introduced a bug where
I put the new unspecs, UNSPEC_COND_SMAX and UNSPEC_COND_SMIN, into the
wrong iterator.

I should have put new unspecs in SVE_COND_FP_MAXMIN but I put it in
SVE_COND_FP_BINARY_REG instead. That was incorrect because the
SVE_COND_FP_MAXMIN iterator is being used for predicated floating-point
maximum/minimum, not SVE_COND_FP_BINARY_REG.

Also added a testcase to validate the new change.

Regression tested on aarch64-unknown-linux-gnu and found no regressions.
There are some test cases with "libitm" in their directory names which
appear in compare_tests output as changed tests but it looks like they
are in the output just because of changed build directories, like from
build-patched/aarch64-unknown-linux-gnu/./libitm/* to
build-pristine/aarch64-unknown-linux-gnu/./libitm/*. I didn't think it
was a cause of concern and have pushed this for review.

gcc/ChangeLog:

	* config/aarch64/iterators.md: Move UNSPEC_COND_SMAX and
	UNSPEC_COND_SMIN to correct iterators.

gcc/testsuite/ChangeLog:

	PR target/116934
	* gcc.target/aarch64/sve2/pr116934.c: New test.
---
 gcc/config/aarch64/iterators.md                  |  8 ++++----
 gcc/testsuite/gcc.target/aarch64/sve2/pr116934.c | 13 +++++++++++++
 2 files changed, 17 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/sve2/pr116934.c

Comments

Richard Sandiford Oct. 4, 2024, 4:20 p.m. UTC | #1 
<saurabh.jha@arm.com> writes:
> In ac4cdf5cb43c0b09e81760e2a1902ceebcf1a135, I introduced a bug where
> I put the new unspecs, UNSPEC_COND_SMAX and UNSPEC_COND_SMIN, into the
> wrong iterator.
>
> I should have put new unspecs in SVE_COND_FP_MAXMIN but I put it in
> SVE_COND_FP_BINARY_REG instead. That was incorrect because the
> SVE_COND_FP_MAXMIN iterator is being used for predicated floating-point
> maximum/minimum, not SVE_COND_FP_BINARY_REG.
>
> Also added a testcase to validate the new change.
>
> Regression tested on aarch64-unknown-linux-gnu and found no regressions.
> There are some test cases with "libitm" in their directory names which
> appear in compare_tests output as changed tests but it looks like they
> are in the output just because of changed build directories, like from
> build-patched/aarch64-unknown-linux-gnu/./libitm/* to
> build-pristine/aarch64-unknown-linux-gnu/./libitm/*. I didn't think it
> was a cause of concern and have pushed this for review.
>
> gcc/ChangeLog:
>
> 	* config/aarch64/iterators.md: Move UNSPEC_COND_SMAX and
> 	UNSPEC_COND_SMIN to correct iterators.
>
> gcc/testsuite/ChangeLog:
>
> 	PR target/116934
> 	* gcc.target/aarch64/sve2/pr116934.c: New test.

OK, thanks.  I see the only effect of the patch is (rightly) to add
back the constant zero alternatives.

Richard

> ---
>  gcc/config/aarch64/iterators.md                  |  8 ++++----
>  gcc/testsuite/gcc.target/aarch64/sve2/pr116934.c | 13 +++++++++++++
>  2 files changed, 17 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/sve2/pr116934.c
>
> diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md
> index 0836dee61c9..fcad236eee9 100644
> --- a/gcc/config/aarch64/iterators.md
> +++ b/gcc/config/aarch64/iterators.md
> @@ -3125,9 +3125,7 @@
>  
>  (define_int_iterator SVE_COND_FP_BINARY_REG
>    [UNSPEC_COND_FDIV
> -   UNSPEC_COND_FMULX
> -   UNSPEC_COND_SMAX
> -   UNSPEC_COND_SMIN])
> +   UNSPEC_COND_FMULX])
>  
>  (define_int_iterator SVE_COND_FCADD [UNSPEC_COND_FCADD90
>  				     UNSPEC_COND_FCADD270])
> @@ -3135,7 +3133,9 @@
>  (define_int_iterator SVE_COND_FP_MAXMIN [UNSPEC_COND_FMAX
>  					 UNSPEC_COND_FMAXNM
>  					 UNSPEC_COND_FMIN
> -					 UNSPEC_COND_FMINNM])
> +					 UNSPEC_COND_FMINNM
> +					 UNSPEC_COND_SMAX
> +					 UNSPEC_COND_SMIN])
>  
>  (define_int_iterator SVE_COND_FP_TERNARY [UNSPEC_COND_FMLA
>  					  UNSPEC_COND_FMLS
> diff --git a/gcc/testsuite/gcc.target/aarch64/sve2/pr116934.c b/gcc/testsuite/gcc.target/aarch64/sve2/pr116934.c
> new file mode 100644
> index 00000000000..94fb96ffa7d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/sve2/pr116934.c
> @@ -0,0 +1,13 @@
> +/* { dg-do compile } */
> +/* { dg-additional-options "-Ofast -mcpu=neoverse-v2" } */
> +
> +int a;
> +float *b;
> +
> +void foo() {
> +  for (; a; a--, b += 4) {
> +    b[0] = b[1] = b[2] = b[2] > 0 ?: 0;
> +    if (b[3] < 0)
> +      b[3] = 0;
> +  }
> +}
Wilco Dijkstra Oct. 4, 2024, 4:23 p.m. UTC | #2 
Hi Saurabh,

This looks good, one little nit:

> gcc/ChangeLog:
>
>         * config/aarch64/iterators.md: Move UNSPEC_COND_SMAX and
>         UNSPEC_COND_SMIN to correct iterators.

This should also have the PR target/116934 before it - it's fine to change it when you commit.

Speaking of which, can we try getting this committed before the weekend so the
benchmark runs will work again?

Cheers,
Wilco
Saurabh Jha Oct. 4, 2024, 7:08 p.m. UTC | #3 
Thanks for the reviews. I made the suggested change in the commit 
message, committed, and pushed it:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=20ce363c557d6458ec3193ab4e7df760fbe34976

Thanks,
Saurabh

On 10/4/2024 10:09 AM, saurabh.jha@arm.com wrote:
> 
> In ac4cdf5cb43c0b09e81760e2a1902ceebcf1a135, I introduced a bug where
> I put the new unspecs, UNSPEC_COND_SMAX and UNSPEC_COND_SMIN, into the
> wrong iterator.
> 
> I should have put new unspecs in SVE_COND_FP_MAXMIN but I put it in
> SVE_COND_FP_BINARY_REG instead. That was incorrect because the
> SVE_COND_FP_MAXMIN iterator is being used for predicated floating-point
> maximum/minimum, not SVE_COND_FP_BINARY_REG.
> 
> Also added a testcase to validate the new change.
> 
> Regression tested on aarch64-unknown-linux-gnu and found no regressions.
> There are some test cases with "libitm" in their directory names which
> appear in compare_tests output as changed tests but it looks like they
> are in the output just because of changed build directories, like from
> build-patched/aarch64-unknown-linux-gnu/./libitm/* to
> build-pristine/aarch64-unknown-linux-gnu/./libitm/*. I didn't think it
> was a cause of concern and have pushed this for review.
> 
> gcc/ChangeLog:
> 
> 	* config/aarch64/iterators.md: Move UNSPEC_COND_SMAX and
> 	UNSPEC_COND_SMIN to correct iterators.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR target/116934
> 	* gcc.target/aarch64/sve2/pr116934.c: New test.
> ---
>   gcc/config/aarch64/iterators.md                  |  8 ++++----
>   gcc/testsuite/gcc.target/aarch64/sve2/pr116934.c | 13 +++++++++++++
>   2 files changed, 17 insertions(+), 4 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.target/aarch64/sve2/pr116934.c
>
diff mbox series

Patch

diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md
index 0836dee61c9..fcad236eee9 100644
--- a/gcc/config/aarch64/iterators.md
+++ b/gcc/config/aarch64/iterators.md
@@ -3125,9 +3125,7 @@ 
 
 (define_int_iterator SVE_COND_FP_BINARY_REG
   [UNSPEC_COND_FDIV
-   UNSPEC_COND_FMULX
-   UNSPEC_COND_SMAX
-   UNSPEC_COND_SMIN])
+   UNSPEC_COND_FMULX])
 
 (define_int_iterator SVE_COND_FCADD [UNSPEC_COND_FCADD90
 				     UNSPEC_COND_FCADD270])
@@ -3135,7 +3133,9 @@ 
 (define_int_iterator SVE_COND_FP_MAXMIN [UNSPEC_COND_FMAX
 					 UNSPEC_COND_FMAXNM
 					 UNSPEC_COND_FMIN
-					 UNSPEC_COND_FMINNM])
+					 UNSPEC_COND_FMINNM
+					 UNSPEC_COND_SMAX
+					 UNSPEC_COND_SMIN])
 
 (define_int_iterator SVE_COND_FP_TERNARY [UNSPEC_COND_FMLA
 					  UNSPEC_COND_FMLS
diff --git a/gcc/testsuite/gcc.target/aarch64/sve2/pr116934.c b/gcc/testsuite/gcc.target/aarch64/sve2/pr116934.c
new file mode 100644
index 00000000000..94fb96ffa7d
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/sve2/pr116934.c
@@ -0,0 +1,13 @@ 
+/* { dg-do compile } */
+/* { dg-additional-options "-Ofast -mcpu=neoverse-v2" } */
+
+int a;
+float *b;
+
+void foo() {
+  for (; a; a--, b += 4) {
+    b[0] = b[1] = b[2] = b[2] > 0 ?: 0;
+    if (b[3] < 0)
+      b[3] = 0;
+  }
+}