[PATCH 1/2] testsuite/unroll-8: Avoid triggering undefined behavior

[PATCH 1/2] testsuite/unroll-8: Avoid triggering undefined behavior

[Palmer Dabbelt]

I was poking around with this test failure and noticed it was exercising
undefined behavior.  The return type doesn't matter for what's being
tested, so just mark it as void.

gcc/testsuite/ChangeLog:

	* gcc.dg/unroll-8.c: Remove UB.
---
I didn't tes this, but it seems trivial enough that I'm just going to
throw it at the bots and hope I'm right.
---
 gcc/testsuite/gcc.dg/unroll-8.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/testsuite/gcc.dg/unroll-8.c b/gcc/testsuite/gcc.dg/unroll-8.c
index 4388f47d4c7..06d32e56893 100644
--- a/gcc/testsuite/gcc.dg/unroll-8.c
+++ b/gcc/testsuite/gcc.dg/unroll-8.c
@@ -3,7 +3,7 @@
 /* { dg-additional-options "-fno-tree-vectorize" { target amdgcn-*-* } } */
 
 struct a {int a[7];};
-int t(struct a *a, int n)
+void t(struct a *a, int n)
 {
   int i;
   for (i=0;i<n;i++)
-- 
2.42.1

[PATCH 2/2] testsuite/unroll-8: Disable vectorization for varibale-factor targets

[Palmer Dabbelt]

The vectorizer picks up these loops and disables unrolling on targets
with variable vector factors.  That result in better code here, but it
trips up the unrolling tests.  So just disable vectorization for these.

gcc/testsuite/ChangeLog:

	PR target/112531
	* gcc.dg/unroll-8.c: Disable vectorization on arm64 and riscv.
---
This also isn't tested.
---
 gcc/testsuite/gcc.dg/unroll-8.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/gcc/testsuite/gcc.dg/unroll-8.c b/gcc/testsuite/gcc.dg/unroll-8.c
index 06d32e56893..4465c620800 100644
--- a/gcc/testsuite/gcc.dg/unroll-8.c
+++ b/gcc/testsuite/gcc.dg/unroll-8.c
@@ -1,6 +1,15 @@
 /* { dg-do compile } */
 /* { dg-options "-O2 -fdump-rtl-loop2_unroll-details-blocks -funroll-loops" } */
+
+/*
+ * Targets that support variable-length vectorization don't unroll loops (see
+ * the "Disabling unrolling due to variable-length vectorization factor" out in
+ * tree-vect-loop.  So disable tree vectorization for these targets, as it will
+ * interfere with the unrolling we're looking for below.
+ */
+/* { dg-additional-options "-fno-tree-vectorize" { target aarch64-*-* } } */
 /* { dg-additional-options "-fno-tree-vectorize" { target amdgcn-*-* } } */
+/* { dg-additional-options "-fno-tree-vectorize" { target riscv*-*-* } } */
 
 struct a {int a[7];};
 void t(struct a *a, int n)
-- 
2.42.1

Re: [PATCH 1/2] testsuite/unroll-8: Avoid triggering undefined behavior

[Jeff Law]

On 11/21/23 16:27, Palmer Dabbelt wrote:
> I was poking around with this test failure and noticed it was exercising
> undefined behavior.  The return type doesn't matter for what's being
> tested, so just mark it as void.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.dg/unroll-8.c: Remove UB.
I just reviewed the history of unroll-8, I don't think this compromises 
the test's original intent.  OK for the trunk.

jeff

Re: [PATCH 2/2] testsuite/unroll-8: Disable vectorization for varibale-factor targets

[Jeff Law]

On 11/21/23 16:27, Palmer Dabbelt wrote:
> The vectorizer picks up these loops and disables unrolling on targets
> with variable vector factors.  That result in better code here, but it
> trips up the unrolling tests.  So just disable vectorization for these.
> 
> gcc/testsuite/ChangeLog:
> 
> 	PR target/112531
> 	* gcc.dg/unroll-8.c: Disable vectorization on arm64 and riscv.
So probably the right check is to test for vector and 
!vect_variable_length rather than doing something target specific for 
aarch64/riscv

Jeff

Re: [PATCH 1/2] testsuite/unroll-8: Avoid triggering undefined behavior

[Andrew Pinski]

On Tue, Nov 21, 2023 at 3:29 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> I was poking around with this test failure and noticed it was exercising
> undefined behavior.  The return type doesn't matter for what's being
> tested, so just mark it as void.

Just a quick note, this is NOT undefined behavior in C to return
without a value from a function which has a non-void return type. It
is only undefined if the value was used.
It is undefined behavior in C++ though for a fallthrough.
Yes there is a difference in the language. As Jeff said it does not
change what the testcase was/is testing but we should be clear in the
changelog that this is NOT undefined behavior.

Thanks,
Andrew Pinski

>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/unroll-8.c: Remove UB.
> ---
> I didn't tes this, but it seems trivial enough that I'm just going to
> throw it at the bots and hope I'm right.
> ---
>  gcc/testsuite/gcc.dg/unroll-8.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/testsuite/gcc.dg/unroll-8.c b/gcc/testsuite/gcc.dg/unroll-8.c
> index 4388f47d4c7..06d32e56893 100644
> --- a/gcc/testsuite/gcc.dg/unroll-8.c
> +++ b/gcc/testsuite/gcc.dg/unroll-8.c
> @@ -3,7 +3,7 @@
>  /* { dg-additional-options "-fno-tree-vectorize" { target amdgcn-*-* } } */
>
>  struct a {int a[7];};
> -int t(struct a *a, int n)
> +void t(struct a *a, int n)
>  {
>    int i;
>    for (i=0;i<n;i++)
> --
> 2.42.1
>

Re: [PATCH 2/2] testsuite/unroll-8: Disable vectorization for varibale-factor targets

[Andrew Pinski]

On Wed, Nov 22, 2023 at 4:18 PM Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
>
> On 11/21/23 16:27, Palmer Dabbelt wrote:
> > The vectorizer picks up these loops and disables unrolling on targets
> > with variable vector factors.  That result in better code here, but it
> > trips up the unrolling tests.  So just disable vectorization for these.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       PR target/112531
> >       * gcc.dg/unroll-8.c: Disable vectorization on arm64 and riscv.
> So probably the right check is to test for vector and
> !vect_variable_length rather than doing something target specific for
> aarch64/riscv

Yes I think that would definitely be better.

>
> Jeff