diff mbox series

[aarch64] Fix PR94591: GCC generates invalid rev64 insns

Message ID 20200519162536.7e7dwfklqkh5almm@arm.com
State New
Headers show
Series [aarch64] Fix PR94591: GCC generates invalid rev64 insns | expand

Commit Message

Alex Coplan May 19, 2020, 4:25 p.m. UTC 
Hello,

This patch fixes PR94591. The problem was the function aarch64_evpc_rev_local()
matching vector permutations that were not reversals. In particular, prior to
this patch, this function matched the identity permutation which led to
generating bogus REV64 insns which were rejected by the assembler.

Testing:
 - New regression test which passes after applying the patch.
 - New test passes on an x64 -> aarch64-none-elf cross.
 - Bootstrap and regtest on aarch64-linux-gnu.

OK to install?

Thanks,
Alex

---

gcc/ChangeLog:

2020-05-19  Alex Coplan  <alex.coplan@arm.com>

	PR target/94591
	* config/aarch64/aarch64.c (aarch64_evpc_rev_local): Don't match
	identity permutation.

gcc/testsuite/ChangeLog:

2020-05-19  Alex Coplan  <alex.coplan@arm.com>

	PR target/94591
	* gcc.c-torture/execute/pr94591.c: New test.

Comments

Richard Sandiford May 19, 2020, 4:59 p.m. UTC | #1 
Alex Coplan <alex.coplan@arm.com> writes:
> Hello,
>
> This patch fixes PR94591. The problem was the function aarch64_evpc_rev_local()
> matching vector permutations that were not reversals. In particular, prior to
> this patch, this function matched the identity permutation which led to
> generating bogus REV64 insns which were rejected by the assembler.
>
> Testing:
>  - New regression test which passes after applying the patch.
>  - New test passes on an x64 -> aarch64-none-elf cross.
>  - Bootstrap and regtest on aarch64-linux-gnu.
>
> OK to install?
>
> Thanks,
> Alex
>
> ---
>
> gcc/ChangeLog:
>
> 2020-05-19  Alex Coplan  <alex.coplan@arm.com>
>
> 	PR target/94591
> 	* config/aarch64/aarch64.c (aarch64_evpc_rev_local): Don't match
> 	identity permutation.
>
> gcc/testsuite/ChangeLog:
>
> 2020-05-19  Alex Coplan  <alex.coplan@arm.com>
>
> 	PR target/94591
> 	* gcc.c-torture/execute/pr94591.c: New test.

OK, thanks.

Richard

> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 70aa2f752b5..79c016f4dc3 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -20191,7 +20191,8 @@ aarch64_evpc_rev_local (struct expand_vec_perm_d *d)
>  
>    if (d->vec_flags == VEC_SVE_PRED
>        || !d->one_vector_p
> -      || !d->perm[0].is_constant (&diff))
> +      || !d->perm[0].is_constant (&diff)
> +      || !diff)
>      return false;
>  
>    size = (diff + 1) * GET_MODE_UNIT_SIZE (d->vmode);
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr94591.c b/gcc/testsuite/gcc.c-torture/execute/pr94591.c
> new file mode 100644
> index 00000000000..42271ad8bce
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr94591.c
> @@ -0,0 +1,32 @@
> +typedef unsigned __attribute__((__vector_size__(8))) V2SI_u;
> +typedef int __attribute__((__vector_size__(8))) V2SI_d;
> +
> +typedef unsigned long __attribute__((__vector_size__(16))) V2DI_u;
> +typedef long __attribute__((__vector_size__(16))) V2DI_d;
> +
> +void id_V2SI(V2SI_d *v)
> +{
> +  *v = __builtin_shuffle(*v, (V2SI_d)(V2SI_u) { 0, 1 });
> +}
> +
> +void id_V2DI(V2DI_d *v)
> +{
> +  *v = __builtin_shuffle(*v, (V2DI_d)(V2DI_u) { 0, 1 });
> +}
> +
> +extern void abort(void);
> +
> +int main(void)
> +{
> +  V2SI_d si = { 35, 42 };
> +  id_V2SI(&si);
> +
> +  if (si[0] != 35 || si[1] != 42)
> +    abort();
> +
> +  V2DI_d di = { 63, 38 };
> +  id_V2DI(&di);
> +
> +  if (di[0] != 63 || di[1] != 38)
> +    abort();
> +}
Alex Coplan May 28, 2020, 2:47 p.m. UTC | #2 
On 19/05/2020 17:59, Richard Sandiford wrote:
> Alex Coplan <alex.coplan@arm.com> writes:
> > Hello,
> >
> > This patch fixes PR94591. The problem was the function aarch64_evpc_rev_local()
> > matching vector permutations that were not reversals. In particular, prior to
> > this patch, this function matched the identity permutation which led to
> > generating bogus REV64 insns which were rejected by the assembler.
> >
> > Testing:
> >  - New regression test which passes after applying the patch.
> >  - New test passes on an x64 -> aarch64-none-elf cross.
> >  - Bootstrap and regtest on aarch64-linux-gnu.
> >
> > OK to install?
> >
> > Thanks,
> > Alex
> >
> > ---
> >
> > gcc/ChangeLog:
> >
> > 2020-05-19  Alex Coplan  <alex.coplan@arm.com>
> >
> > 	PR target/94591
> > 	* config/aarch64/aarch64.c (aarch64_evpc_rev_local): Don't match
> > 	identity permutation.
> >
> > gcc/testsuite/ChangeLog:
> >
> > 2020-05-19  Alex Coplan  <alex.coplan@arm.com>
> >
> > 	PR target/94591
> > 	* gcc.c-torture/execute/pr94591.c: New test.
> 
> OK, thanks.
> 
> Richard

I've just tested this patch on gcc-{8,9,10} release branches:
bootstraps+regtests on aarch64-linux-gnu came back clean.

Since this was a regression introduced in GCC 8, is it OK to backport
the fix to those release branches now?

Thanks,
Alex

> 
> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > index 70aa2f752b5..79c016f4dc3 100644
> > --- a/gcc/config/aarch64/aarch64.c
> > +++ b/gcc/config/aarch64/aarch64.c
> > @@ -20191,7 +20191,8 @@ aarch64_evpc_rev_local (struct expand_vec_perm_d *d)
> >  
> >    if (d->vec_flags == VEC_SVE_PRED
> >        || !d->one_vector_p
> > -      || !d->perm[0].is_constant (&diff))
> > +      || !d->perm[0].is_constant (&diff)
> > +      || !diff)
> >      return false;
> >  
> >    size = (diff + 1) * GET_MODE_UNIT_SIZE (d->vmode);
> > diff --git a/gcc/testsuite/gcc.c-torture/execute/pr94591.c b/gcc/testsuite/gcc.c-torture/execute/pr94591.c
> > new file mode 100644
> > index 00000000000..42271ad8bce
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/execute/pr94591.c
> > @@ -0,0 +1,32 @@
> > +typedef unsigned __attribute__((__vector_size__(8))) V2SI_u;
> > +typedef int __attribute__((__vector_size__(8))) V2SI_d;
> > +
> > +typedef unsigned long __attribute__((__vector_size__(16))) V2DI_u;
> > +typedef long __attribute__((__vector_size__(16))) V2DI_d;
> > +
> > +void id_V2SI(V2SI_d *v)
> > +{
> > +  *v = __builtin_shuffle(*v, (V2SI_d)(V2SI_u) { 0, 1 });
> > +}
> > +
> > +void id_V2DI(V2DI_d *v)
> > +{
> > +  *v = __builtin_shuffle(*v, (V2DI_d)(V2DI_u) { 0, 1 });
> > +}
> > +
> > +extern void abort(void);
> > +
> > +int main(void)
> > +{
> > +  V2SI_d si = { 35, 42 };
> > +  id_V2SI(&si);
> > +
> > +  if (si[0] != 35 || si[1] != 42)
> > +    abort();
> > +
> > +  V2DI_d di = { 63, 38 };
> > +  id_V2DI(&di);
> > +
> > +  if (di[0] != 63 || di[1] != 38)
> > +    abort();
> > +}
Richard Sandiford May 29, 2020, 10:59 a.m. UTC | #3 
Alex Coplan <alex.coplan@arm.com> writes:
> On 19/05/2020 17:59, Richard Sandiford wrote:
>> Alex Coplan <alex.coplan@arm.com> writes:
>> > Hello,
>> >
>> > This patch fixes PR94591. The problem was the function aarch64_evpc_rev_local()
>> > matching vector permutations that were not reversals. In particular, prior to
>> > this patch, this function matched the identity permutation which led to
>> > generating bogus REV64 insns which were rejected by the assembler.
>> >
>> > Testing:
>> >  - New regression test which passes after applying the patch.
>> >  - New test passes on an x64 -> aarch64-none-elf cross.
>> >  - Bootstrap and regtest on aarch64-linux-gnu.
>> >
>> > OK to install?
>> >
>> > Thanks,
>> > Alex
>> >
>> > ---
>> >
>> > gcc/ChangeLog:
>> >
>> > 2020-05-19  Alex Coplan  <alex.coplan@arm.com>
>> >
>> > 	PR target/94591
>> > 	* config/aarch64/aarch64.c (aarch64_evpc_rev_local): Don't match
>> > 	identity permutation.
>> >
>> > gcc/testsuite/ChangeLog:
>> >
>> > 2020-05-19  Alex Coplan  <alex.coplan@arm.com>
>> >
>> > 	PR target/94591
>> > 	* gcc.c-torture/execute/pr94591.c: New test.
>> 
>> OK, thanks.
>> 
>> Richard
>
> I've just tested this patch on gcc-{8,9,10} release branches:
> bootstraps+regtests on aarch64-linux-gnu came back clean.
>
> Since this was a regression introduced in GCC 8, is it OK to backport
> the fix to those release branches now?

Yeah, OK for the branches too.

Thanks,
Richard
Alex Coplan May 29, 2020, 1:27 p.m. UTC | #4 
> -----Original Message-----
> From: Richard Sandiford <richard.sandiford@arm.com>
> Sent: 29 May 2020 11:59
> To: Alex Coplan <Alex.Coplan@arm.com>
> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; Richard Earnshaw
> <Richard.Earnshaw@arm.com>; Marcus Shawcroft <Marcus.Shawcroft@arm.com>;
> Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> Subject: Re: [PATCH] [aarch64] Fix PR94591: GCC generates invalid rev64
> insns
> 
> Alex Coplan <alex.coplan@arm.com> writes:
> > On 19/05/2020 17:59, Richard Sandiford wrote:
> >> Alex Coplan <alex.coplan@arm.com> writes:
> >> > Hello,
> >> >
> >> > This patch fixes PR94591. The problem was the function
> aarch64_evpc_rev_local()
> >> > matching vector permutations that were not reversals. In particular,
> prior to
> >> > this patch, this function matched the identity permutation which led
> to
> >> > generating bogus REV64 insns which were rejected by the assembler.
> >> >
> >> > Testing:
> >> >  - New regression test which passes after applying the patch.
> >> >  - New test passes on an x64 -> aarch64-none-elf cross.
> >> >  - Bootstrap and regtest on aarch64-linux-gnu.
> >> >
> >> > OK to install?
> >> >
> >> > Thanks,
> >> > Alex
> >> >
> >> > ---
> >> >
> >> > gcc/ChangeLog:
> >> >
> >> > 2020-05-19  Alex Coplan  <alex.coplan@arm.com>
> >> >
> >> > 	PR target/94591
> >> > 	* config/aarch64/aarch64.c (aarch64_evpc_rev_local): Don't match
> >> > 	identity permutation.
> >> >
> >> > gcc/testsuite/ChangeLog:
> >> >
> >> > 2020-05-19  Alex Coplan  <alex.coplan@arm.com>
> >> >
> >> > 	PR target/94591
> >> > 	* gcc.c-torture/execute/pr94591.c: New test.
> >>
> >> OK, thanks.
> >>
> >> Richard
> >
> > I've just tested this patch on gcc-{8,9,10} release branches:
> > bootstraps+regtests on aarch64-linux-gnu came back clean.
> >
> > Since this was a regression introduced in GCC 8, is it OK to backport
> > the fix to those release branches now?
> 
> Yeah, OK for the branches too.

Installed on the branches.

Thanks,
Alex
diff mbox series

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 70aa2f752b5..79c016f4dc3 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -20191,7 +20191,8 @@  aarch64_evpc_rev_local (struct expand_vec_perm_d *d)
 
   if (d->vec_flags == VEC_SVE_PRED
       || !d->one_vector_p
-      || !d->perm[0].is_constant (&diff))
+      || !d->perm[0].is_constant (&diff)
+      || !diff)
     return false;
 
   size = (diff + 1) * GET_MODE_UNIT_SIZE (d->vmode);
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr94591.c b/gcc/testsuite/gcc.c-torture/execute/pr94591.c
new file mode 100644
index 00000000000..42271ad8bce
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr94591.c
@@ -0,0 +1,32 @@ 
+typedef unsigned __attribute__((__vector_size__(8))) V2SI_u;
+typedef int __attribute__((__vector_size__(8))) V2SI_d;
+
+typedef unsigned long __attribute__((__vector_size__(16))) V2DI_u;
+typedef long __attribute__((__vector_size__(16))) V2DI_d;
+
+void id_V2SI(V2SI_d *v)
+{
+  *v = __builtin_shuffle(*v, (V2SI_d)(V2SI_u) { 0, 1 });
+}
+
+void id_V2DI(V2DI_d *v)
+{
+  *v = __builtin_shuffle(*v, (V2DI_d)(V2DI_u) { 0, 1 });
+}
+
+extern void abort(void);
+
+int main(void)
+{
+  V2SI_d si = { 35, 42 };
+  id_V2SI(&si);
+
+  if (si[0] != 35 || si[1] != 42)
+    abort();
+
+  V2DI_d di = { 63, 38 };
+  id_V2DI(&di);
+
+  if (di[0] != 63 || di[1] != 38)
+    abort();
+}