RE: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR 107674]
> -Original Message-
> From: Andre Vieira (lists)
> Sent: Monday, January 30, 2023 4:39 PM
> To: Kyrylo Tkachov ; gcc-patches@gcc.gnu.org
> Cc: Richard Earnshaw
> Subject: Re: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR
> 107674]
>
> Here's a new version with a more robust test.
>
> OK for trunk?
Ok.
Thanks,
Kyrill
>
> On 27/01/2023 09:56, Kyrylo Tkachov wrote:
>
> >
> >
> >> -Original Message-
> >> From: Andre Vieira (lists)
> >> Sent: Friday, January 27, 2023 9:54 AM
> >> To: Kyrylo Tkachov ; gcc-patches@gcc.gnu.org
> >> Cc: Richard Earnshaw
> >> Subject: Re: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR
> >> 107674]
> >>
> >>
> >>
> >> On 26/01/2023 15:02, Kyrylo Tkachov wrote:
> >>> Hi Andre,
> >>>
> >>>> -Original Message-
> >>>> From: Andre Vieira (lists)
> >>>> Sent: Tuesday, January 24, 2023 1:41 PM
> >>>> To: gcc-patches@gcc.gnu.org
> >>>> Cc: Kyrylo Tkachov ; Richard Earnshaw
> >>>>
> >>>> Subject: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR
> >>>> 107674]
> >>>>
> >>>> Hi,
> >>>>
> >>>> The ACLE defines mve_pred16_t as an unsigned short. This patch
> makes
> >>>> sure GCC treats the predicate as an unsigned type, rather than signed.
> >>>>
> >>>> Bootstrapped on aarch64-none-eabi and regression tested on arm-
> none-
> >> eabi
> >>>> and armeb-none-eabi for armv8.1-m.main+mve.fp.
> >>>>
> >>>> OK for trunk?
> >>>>
> >>>> gcc/ChangeLog:
> >>>>
> >>>> PR target/107674
> >>>> * config/arm/arm-builtins.cc (arm_simd_builtin_type): Rewrite to
> >>>> use
> >>>> new qualifiers parameter and use unsigned short type for MVE
> >>>> predicate.
> >>>> (arm_init_builtin): Call arm_simd_builtin_type with qualifiers
> >>>> parameter.
> >>>> (arm_init_crypto_builtins): Likewise.
> >>>>
> >>>> gcc/testsuite/ChangeLog:
> >>>>
> >>>> PR target/107674
> >>>> * gcc.target/arm/mve/mve_vpt.c: New test.
> >>>
> >>> diff --git a/gcc/config/arm/arm-builtins.cc b/gcc/config/arm/arm-
> builtins.cc
> >>> index
> >>
> 11d7478d9df69139802a9d42c09dd0de7480b60e..6c67cec93ff76a4b42f3a0b3
> >> 05f697142e88fcd9 100644
> >>> --- a/gcc/config/arm/arm-builtins.cc
> >>> +++ b/gcc/config/arm/arm-builtins.cc
> >>> @@ -1489,12 +1489,14 @@ arm_lookup_simd_builtin_type
> >> (machine_mode mode,
> >>>}
> >>>
> >>>static tree
> >>> -arm_simd_builtin_type (machine_mode mode, bool unsigned_p, bool
> >> poly_p)
> >>> +arm_simd_builtin_type (machine_mode mode, enum
> arm_type_qualifiers
> >> qualifiers)
> >>>{
> >>>
> >>> I think in C++ now we can leave out the "enum" here.
> >>>
> >>> diff --git a/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
> >> b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
> >>> new file mode 100644
> >>> index
> >>
> ..26a565b79dd1348e361b3a
> >> a23a1d6e6d13bffce8
> >>> --- /dev/null
> >>> +++ b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
> >>> @@ -0,0 +1,27 @@
> >>> +/* { dg-options "-O2" } */
> >>> +/* { dg-require-effective-target arm_v8_1m_mve_ok } */
> >>> +/* { dg-add-options arm_v8_1m_mve } */
> >>> +/* { dg-final { check-function-bodies "**" "" } } */
> >>> +#include
> >>> +void test0 (uint8_t *a, uint8_t *b, uint8_t *c)
> >>> +{
> >>> +uint8x16_t va = vldrbq_u8 (a);
> >>> +uint8x16_t vb = vldrbq_u8 (b);
> >>> +mve_pred16_t p = vcmpeqq_u8 (va, vb);
> >>> +uint8x16_t vc = vaddq_x_u8 (va, vb, p);
> >>> +vstrbq_p_u8 (c, vc, p);
> >>> +}
> >>> +/*
> >>> +** test0:
> >>> +** vldrb.8 q2, \[r0\]
> >>> +** vldrb.8 q1, \[r1\]
> >>> +** vcmp.i8 eq, q2, q1
> >>> +** vmrsr3, p0 @ movhi
> >>> +** uxthr3, r3
> >>> +** vmsrp0, r3 @ movhi
> >>> +** vpst
> >>> +** vaddt.i8q3, q2, q1
> >>> +** vpst
> >>> +** vstrbt.8q3, \[r2\]
> >>> +** bx lr
> >>> +*/
> >>>
> >>> This explicit assembly matching looks quite fragile and sensitive to
> >>> future
> >> scheduling and RA changes.
> >>> Is there something more targeted we could scan for to check that the
> >> predicate is unsigned now?
> >> No not really, as it's not unsigned everywhere, only in its intermediate
> >> representation between intrinsics. GCC is aware that mve_pred16_t is an
> >> unsigned short, so as soon as you try to use the value on its own or
> >> pass it as an argument or return, there is an implicit cast.
> >>
> >> I could make this particular test-case more robust by not checking
> >> specific registers. Though the sequence of loads-cmp-add-store will
> >> always be the same as it's data-dependent.
> >
> > Yeah, I suspected as such. Ok, let's abstract the registers away (I think
> check-function-bodies can use regex capturing to record particular registers)
> then.
> > Thanks,
> > Kyrill
> >
Re: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR 107674]
Here's a new version with a more robust test.
OK for trunk?
On 27/01/2023 09:56, Kyrylo Tkachov wrote:
-Original Message-
From: Andre Vieira (lists)
Sent: Friday, January 27, 2023 9:54 AM
To: Kyrylo Tkachov ; gcc-patches@gcc.gnu.org
Cc: Richard Earnshaw
Subject: Re: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR
107674]
On 26/01/2023 15:02, Kyrylo Tkachov wrote:
Hi Andre,
-Original Message-
From: Andre Vieira (lists)
Sent: Tuesday, January 24, 2023 1:41 PM
To: gcc-patches@gcc.gnu.org
Cc: Kyrylo Tkachov ; Richard Earnshaw
Subject: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR
107674]
Hi,
The ACLE defines mve_pred16_t as an unsigned short. This patch makes
sure GCC treats the predicate as an unsigned type, rather than signed.
Bootstrapped on aarch64-none-eabi and regression tested on arm-none-
eabi
and armeb-none-eabi for armv8.1-m.main+mve.fp.
OK for trunk?
gcc/ChangeLog:
PR target/107674
* config/arm/arm-builtins.cc (arm_simd_builtin_type): Rewrite to
use
new qualifiers parameter and use unsigned short type for MVE
predicate.
(arm_init_builtin): Call arm_simd_builtin_type with qualifiers
parameter.
(arm_init_crypto_builtins): Likewise.
gcc/testsuite/ChangeLog:
PR target/107674
* gcc.target/arm/mve/mve_vpt.c: New test.
diff --git a/gcc/config/arm/arm-builtins.cc b/gcc/config/arm/arm-builtins.cc
index
11d7478d9df69139802a9d42c09dd0de7480b60e..6c67cec93ff76a4b42f3a0b3
05f697142e88fcd9 100644
--- a/gcc/config/arm/arm-builtins.cc
+++ b/gcc/config/arm/arm-builtins.cc
@@ -1489,12 +1489,14 @@ arm_lookup_simd_builtin_type
(machine_mode mode,
}
static tree
-arm_simd_builtin_type (machine_mode mode, bool unsigned_p, bool
poly_p)
+arm_simd_builtin_type (machine_mode mode, enum arm_type_qualifiers
qualifiers)
{
I think in C++ now we can leave out the "enum" here.
diff --git a/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
new file mode 100644
index
..26a565b79dd1348e361b3a
a23a1d6e6d13bffce8
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
@@ -0,0 +1,27 @@
+/* { dg-options "-O2" } */
+/* { dg-require-effective-target arm_v8_1m_mve_ok } */
+/* { dg-add-options arm_v8_1m_mve } */
+/* { dg-final { check-function-bodies "**" "" } } */
+#include
+void test0 (uint8_t *a, uint8_t *b, uint8_t *c)
+{
+uint8x16_t va = vldrbq_u8 (a);
+uint8x16_t vb = vldrbq_u8 (b);
+mve_pred16_t p = vcmpeqq_u8 (va, vb);
+uint8x16_t vc = vaddq_x_u8 (va, vb, p);
+vstrbq_p_u8 (c, vc, p);
+}
+/*
+** test0:
+** vldrb.8 q2, \[r0\]
+** vldrb.8 q1, \[r1\]
+** vcmp.i8 eq, q2, q1
+** vmrsr3, p0 @ movhi
+** uxthr3, r3
+** vmsrp0, r3 @ movhi
+** vpst
+** vaddt.i8q3, q2, q1
+** vpst
+** vstrbt.8q3, \[r2\]
+** bx lr
+*/
This explicit assembly matching looks quite fragile and sensitive to future
scheduling and RA changes.
Is there something more targeted we could scan for to check that the
predicate is unsigned now?
No not really, as it's not unsigned everywhere, only in its intermediate
representation between intrinsics. GCC is aware that mve_pred16_t is an
unsigned short, so as soon as you try to use the value on its own or
pass it as an argument or return, there is an implicit cast.
I could make this particular test-case more robust by not checking
specific registers. Though the sequence of loads-cmp-add-store will
always be the same as it's data-dependent.
Yeah, I suspected as such. Ok, let's abstract the registers away (I think
check-function-bodies can use regex capturing to record particular registers)
then.
Thanks,
Kyrill
diff --git a/gcc/config/arm/arm-builtins.cc b/gcc/config/arm/arm-builtins.cc
index
11d7478d9df69139802a9d42c09dd0de7480b60e..58bf856f08d8d4836d01c5e4545dc5bab2f39c14
100644
--- a/gcc/config/arm/arm-builtins.cc
+++ b/gcc/config/arm/arm-builtins.cc
@@ -1489,12 +1489,14 @@ arm_lookup_simd_builtin_type (machine_mode mode,
}
static tree
-arm_simd_builtin_type (machine_mode mode, bool unsigned_p, bool poly_p)
+arm_simd_builtin_type (machine_mode mode, arm_type_qualifiers qualifiers)
{
- if (poly_p)
+ if ((qualifiers & qualifier_poly) != 0)
return arm_lookup_simd_builtin_type (mode, qualifier_poly);
- else if (unsigned_p)
+ else if ((qualifiers & qualifier_unsigned) != 0)
return arm_lookup_simd_builtin_type (mode, qualifier_unsigned);
+ else if ((qualifiers & qualifier_predicate) != 0)
+return unsigned_intHI_type_node;
else
return arm_lookup_simd_builtin_type (mode, qualifier_none);
}
@@ -1755,9 +1757,7 @@ arm_init_builtin (unsigned int fcode, arm_builtin_datum
*d,
else
{
eltype
- = arm_simd_builtin_type (op_mode,
-
RE: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR 107674]
> -Original Message-
> From: Andre Vieira (lists)
> Sent: Friday, January 27, 2023 9:54 AM
> To: Kyrylo Tkachov ; gcc-patches@gcc.gnu.org
> Cc: Richard Earnshaw
> Subject: Re: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR
> 107674]
>
>
>
> On 26/01/2023 15:02, Kyrylo Tkachov wrote:
> > Hi Andre,
> >
> >> -Original Message-
> >> From: Andre Vieira (lists)
> >> Sent: Tuesday, January 24, 2023 1:41 PM
> >> To: gcc-patches@gcc.gnu.org
> >> Cc: Kyrylo Tkachov ; Richard Earnshaw
> >>
> >> Subject: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR
> >> 107674]
> >>
> >> Hi,
> >>
> >> The ACLE defines mve_pred16_t as an unsigned short. This patch makes
> >> sure GCC treats the predicate as an unsigned type, rather than signed.
> >>
> >> Bootstrapped on aarch64-none-eabi and regression tested on arm-none-
> eabi
> >> and armeb-none-eabi for armv8.1-m.main+mve.fp.
> >>
> >> OK for trunk?
> >>
> >> gcc/ChangeLog:
> >>
> >>PR target/107674
> >>* config/arm/arm-builtins.cc (arm_simd_builtin_type): Rewrite to
> >> use
> >>new qualifiers parameter and use unsigned short type for MVE
> >> predicate.
> >>(arm_init_builtin): Call arm_simd_builtin_type with qualifiers
> >>parameter.
> >>(arm_init_crypto_builtins): Likewise.
> >>
> >> gcc/testsuite/ChangeLog:
> >>
> >>PR target/107674
> >>* gcc.target/arm/mve/mve_vpt.c: New test.
> >
> > diff --git a/gcc/config/arm/arm-builtins.cc b/gcc/config/arm/arm-builtins.cc
> > index
> 11d7478d9df69139802a9d42c09dd0de7480b60e..6c67cec93ff76a4b42f3a0b3
> 05f697142e88fcd9 100644
> > --- a/gcc/config/arm/arm-builtins.cc
> > +++ b/gcc/config/arm/arm-builtins.cc
> > @@ -1489,12 +1489,14 @@ arm_lookup_simd_builtin_type
> (machine_mode mode,
> > }
> >
> > static tree
> > -arm_simd_builtin_type (machine_mode mode, bool unsigned_p, bool
> poly_p)
> > +arm_simd_builtin_type (machine_mode mode, enum arm_type_qualifiers
> qualifiers)
> > {
> >
> > I think in C++ now we can leave out the "enum" here.
> >
> > diff --git a/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
> b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
> > new file mode 100644
> > index
> ..26a565b79dd1348e361b3a
> a23a1d6e6d13bffce8
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
> > @@ -0,0 +1,27 @@
> > +/* { dg-options "-O2" } */
> > +/* { dg-require-effective-target arm_v8_1m_mve_ok } */
> > +/* { dg-add-options arm_v8_1m_mve } */
> > +/* { dg-final { check-function-bodies "**" "" } } */
> > +#include
> > +void test0 (uint8_t *a, uint8_t *b, uint8_t *c)
> > +{
> > +uint8x16_t va = vldrbq_u8 (a);
> > +uint8x16_t vb = vldrbq_u8 (b);
> > +mve_pred16_t p = vcmpeqq_u8 (va, vb);
> > +uint8x16_t vc = vaddq_x_u8 (va, vb, p);
> > +vstrbq_p_u8 (c, vc, p);
> > +}
> > +/*
> > +** test0:
> > +** vldrb.8 q2, \[r0\]
> > +** vldrb.8 q1, \[r1\]
> > +** vcmp.i8 eq, q2, q1
> > +** vmrsr3, p0 @ movhi
> > +** uxthr3, r3
> > +** vmsrp0, r3 @ movhi
> > +** vpst
> > +** vaddt.i8q3, q2, q1
> > +** vpst
> > +** vstrbt.8q3, \[r2\]
> > +** bx lr
> > +*/
> >
> > This explicit assembly matching looks quite fragile and sensitive to future
> scheduling and RA changes.
> > Is there something more targeted we could scan for to check that the
> predicate is unsigned now?
> No not really, as it's not unsigned everywhere, only in its intermediate
> representation between intrinsics. GCC is aware that mve_pred16_t is an
> unsigned short, so as soon as you try to use the value on its own or
> pass it as an argument or return, there is an implicit cast.
>
> I could make this particular test-case more robust by not checking
> specific registers. Though the sequence of loads-cmp-add-store will
> always be the same as it's data-dependent.
Yeah, I suspected as such. Ok, let's abstract the registers away (I think
check-function-bodies can use regex capturing to record particular registers)
then.
Thanks,
Kyrill
Re: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR 107674]
On 26/01/2023 15:02, Kyrylo Tkachov wrote:
Hi Andre,
-Original Message-
From: Andre Vieira (lists)
Sent: Tuesday, January 24, 2023 1:41 PM
To: gcc-patches@gcc.gnu.org
Cc: Kyrylo Tkachov ; Richard Earnshaw
Subject: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR
107674]
Hi,
The ACLE defines mve_pred16_t as an unsigned short. This patch makes
sure GCC treats the predicate as an unsigned type, rather than signed.
Bootstrapped on aarch64-none-eabi and regression tested on arm-none-eabi
and armeb-none-eabi for armv8.1-m.main+mve.fp.
OK for trunk?
gcc/ChangeLog:
PR target/107674
* config/arm/arm-builtins.cc (arm_simd_builtin_type): Rewrite to
use
new qualifiers parameter and use unsigned short type for MVE
predicate.
(arm_init_builtin): Call arm_simd_builtin_type with qualifiers
parameter.
(arm_init_crypto_builtins): Likewise.
gcc/testsuite/ChangeLog:
PR target/107674
* gcc.target/arm/mve/mve_vpt.c: New test.
diff --git a/gcc/config/arm/arm-builtins.cc b/gcc/config/arm/arm-builtins.cc
index
11d7478d9df69139802a9d42c09dd0de7480b60e..6c67cec93ff76a4b42f3a0b305f697142e88fcd9
100644
--- a/gcc/config/arm/arm-builtins.cc
+++ b/gcc/config/arm/arm-builtins.cc
@@ -1489,12 +1489,14 @@ arm_lookup_simd_builtin_type (machine_mode mode,
}
static tree
-arm_simd_builtin_type (machine_mode mode, bool unsigned_p, bool poly_p)
+arm_simd_builtin_type (machine_mode mode, enum arm_type_qualifiers qualifiers)
{
I think in C++ now we can leave out the "enum" here.
diff --git a/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
new file mode 100644
index
..26a565b79dd1348e361b3aa23a1d6e6d13bffce8
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
@@ -0,0 +1,27 @@
+/* { dg-options "-O2" } */
+/* { dg-require-effective-target arm_v8_1m_mve_ok } */
+/* { dg-add-options arm_v8_1m_mve } */
+/* { dg-final { check-function-bodies "**" "" } } */
+#include
+void test0 (uint8_t *a, uint8_t *b, uint8_t *c)
+{
+uint8x16_t va = vldrbq_u8 (a);
+uint8x16_t vb = vldrbq_u8 (b);
+mve_pred16_t p = vcmpeqq_u8 (va, vb);
+uint8x16_t vc = vaddq_x_u8 (va, vb, p);
+vstrbq_p_u8 (c, vc, p);
+}
+/*
+** test0:
+** vldrb.8 q2, \[r0\]
+** vldrb.8 q1, \[r1\]
+** vcmp.i8 eq, q2, q1
+** vmrsr3, p0 @ movhi
+** uxthr3, r3
+** vmsrp0, r3 @ movhi
+** vpst
+** vaddt.i8q3, q2, q1
+** vpst
+** vstrbt.8q3, \[r2\]
+** bx lr
+*/
This explicit assembly matching looks quite fragile and sensitive to future
scheduling and RA changes.
Is there something more targeted we could scan for to check that the predicate
is unsigned now?
No not really, as it's not unsigned everywhere, only in its intermediate
representation between intrinsics. GCC is aware that mve_pred16_t is an
unsigned short, so as soon as you try to use the value on its own or
pass it as an argument or return, there is an implicit cast.
I could make this particular test-case more robust by not checking
specific registers. Though the sequence of loads-cmp-add-store will
always be the same as it's data-dependent.
RE: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR 107674]
> -Original Message-
> From: Kyrylo Tkachov
> Sent: Thursday, January 26, 2023 3:02 PM
> To: Andre Vieira (lists) ; gcc-
> patc...@gcc.gnu.org
> Cc: Richard Earnshaw
> Subject: RE: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR
> 107674]
>
> Hi Andre,
>
> > -Original Message-
> > From: Andre Vieira (lists)
> > Sent: Tuesday, January 24, 2023 1:41 PM
> > To: gcc-patches@gcc.gnu.org
> > Cc: Kyrylo Tkachov ; Richard Earnshaw
> >
> > Subject: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR
> > 107674]
> >
> > Hi,
> >
> > The ACLE defines mve_pred16_t as an unsigned short. This patch makes
> > sure GCC treats the predicate as an unsigned type, rather than signed.
> >
> > Bootstrapped on aarch64-none-eabi and regression tested on arm-none-
> eabi
> > and armeb-none-eabi for armv8.1-m.main+mve.fp.
> >
> > OK for trunk?
> >
> > gcc/ChangeLog:
> >
> > PR target/107674
> > * config/arm/arm-builtins.cc (arm_simd_builtin_type): Rewrite to
> > use
> > new qualifiers parameter and use unsigned short type for MVE
> > predicate.
> > (arm_init_builtin): Call arm_simd_builtin_type with qualifiers
> > parameter.
> > (arm_init_crypto_builtins): Likewise.
> >
> > gcc/testsuite/ChangeLog:
> >
> > PR target/107674
> > * gcc.target/arm/mve/mve_vpt.c: New test.
>
> diff --git a/gcc/config/arm/arm-builtins.cc b/gcc/config/arm/arm-builtins.cc
> index
> 11d7478d9df69139802a9d42c09dd0de7480b60e..6c67cec93ff76a4b42f3a0b3
> 05f697142e88fcd9 100644
> --- a/gcc/config/arm/arm-builtins.cc
> +++ b/gcc/config/arm/arm-builtins.cc
> @@ -1489,12 +1489,14 @@ arm_lookup_simd_builtin_type (machine_mode
> mode,
> }
>
> static tree
> -arm_simd_builtin_type (machine_mode mode, bool unsigned_p, bool
> poly_p)
> +arm_simd_builtin_type (machine_mode mode, enum arm_type_qualifiers
> qualifiers)
> {
>
> I think in C++ now we can leave out the "enum" here.
>
> diff --git a/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
> b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
> new file mode 100644
> index
> ..26a565b79dd1348e361b3a
> a23a1d6e6d13bffce8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
> @@ -0,0 +1,27 @@
> +/* { dg-options "-O2" } */
> +/* { dg-require-effective-target arm_v8_1m_mve_ok } */
> +/* { dg-add-options arm_v8_1m_mve } */
> +/* { dg-final { check-function-bodies "**" "" } } */
> +#include
> +void test0 (uint8_t *a, uint8_t *b, uint8_t *c)
> +{
> +uint8x16_t va = vldrbq_u8 (a);
> +uint8x16_t vb = vldrbq_u8 (b);
> +mve_pred16_t p = vcmpeqq_u8 (va, vb);
> +uint8x16_t vc = vaddq_x_u8 (va, vb, p);
> +vstrbq_p_u8 (c, vc, p);
> +}
> +/*
> +** test0:
> +** vldrb.8 q2, \[r0\]
> +** vldrb.8 q1, \[r1\]
> +** vcmp.i8 eq, q2, q1
> +** vmrsr3, p0 @ movhi
> +** uxthr3, r3
> +** vmsrp0, r3 @ movhi
> +** vpst
> +** vaddt.i8q3, q2, q1
> +** vpst
> +** vstrbt.8q3, \[r2\]
> +** bx lr
> +*/
>
> This explicit assembly matching looks quite fragile and sensitive to future
> scheduling and RA changes.
> Is there something more targeted we could scan for to check that the
> predicate is unsigned now?
The patch looks fine to me btw. With a more robust testcase and the cosmetic
fix above it can go in.
Thanks,
Kyrill
>
> Thanks,
> Kyrill
RE: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR 107674]
Hi Andre,
> -Original Message-
> From: Andre Vieira (lists)
> Sent: Tuesday, January 24, 2023 1:41 PM
> To: gcc-patches@gcc.gnu.org
> Cc: Kyrylo Tkachov ; Richard Earnshaw
>
> Subject: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR
> 107674]
>
> Hi,
>
> The ACLE defines mve_pred16_t as an unsigned short. This patch makes
> sure GCC treats the predicate as an unsigned type, rather than signed.
>
> Bootstrapped on aarch64-none-eabi and regression tested on arm-none-eabi
> and armeb-none-eabi for armv8.1-m.main+mve.fp.
>
> OK for trunk?
>
> gcc/ChangeLog:
>
> PR target/107674
> * config/arm/arm-builtins.cc (arm_simd_builtin_type): Rewrite to
> use
> new qualifiers parameter and use unsigned short type for MVE
> predicate.
> (arm_init_builtin): Call arm_simd_builtin_type with qualifiers
> parameter.
> (arm_init_crypto_builtins): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> PR target/107674
> * gcc.target/arm/mve/mve_vpt.c: New test.
diff --git a/gcc/config/arm/arm-builtins.cc b/gcc/config/arm/arm-builtins.cc
index
11d7478d9df69139802a9d42c09dd0de7480b60e..6c67cec93ff76a4b42f3a0b305f697142e88fcd9
100644
--- a/gcc/config/arm/arm-builtins.cc
+++ b/gcc/config/arm/arm-builtins.cc
@@ -1489,12 +1489,14 @@ arm_lookup_simd_builtin_type (machine_mode mode,
}
static tree
-arm_simd_builtin_type (machine_mode mode, bool unsigned_p, bool poly_p)
+arm_simd_builtin_type (machine_mode mode, enum arm_type_qualifiers qualifiers)
{
I think in C++ now we can leave out the "enum" here.
diff --git a/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
new file mode 100644
index
..26a565b79dd1348e361b3aa23a1d6e6d13bffce8
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
@@ -0,0 +1,27 @@
+/* { dg-options "-O2" } */
+/* { dg-require-effective-target arm_v8_1m_mve_ok } */
+/* { dg-add-options arm_v8_1m_mve } */
+/* { dg-final { check-function-bodies "**" "" } } */
+#include
+void test0 (uint8_t *a, uint8_t *b, uint8_t *c)
+{
+uint8x16_t va = vldrbq_u8 (a);
+uint8x16_t vb = vldrbq_u8 (b);
+mve_pred16_t p = vcmpeqq_u8 (va, vb);
+uint8x16_t vc = vaddq_x_u8 (va, vb, p);
+vstrbq_p_u8 (c, vc, p);
+}
+/*
+** test0:
+** vldrb.8 q2, \[r0\]
+** vldrb.8 q1, \[r1\]
+** vcmp.i8 eq, q2, q1
+** vmrsr3, p0 @ movhi
+** uxthr3, r3
+** vmsrp0, r3 @ movhi
+** vpst
+** vaddt.i8q3, q2, q1
+** vpst
+** vstrbt.8q3, \[r2\]
+** bx lr
+*/
This explicit assembly matching looks quite fragile and sensitive to future
scheduling and RA changes.
Is there something more targeted we could scan for to check that the predicate
is unsigned now?
Thanks,
Kyrill
Re: [PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR 107674]
I meant bootstrapped on aarch64-none-linux-gnu and not none-eabi. On 24/01/2023 13:40, Andre Vieira (lists) via Gcc-patches wrote: Hi, The ACLE defines mve_pred16_t as an unsigned short. This patch makes sure GCC treats the predicate as an unsigned type, rather than signed. Bootstrapped on aarch64-none-eabi and regression tested on arm-none-eabi and armeb-none-eabi for armv8.1-m.main+mve.fp. OK for trunk? gcc/ChangeLog: PR target/107674 * config/arm/arm-builtins.cc (arm_simd_builtin_type): Rewrite to use new qualifiers parameter and use unsigned short type for MVE predicate. (arm_init_builtin): Call arm_simd_builtin_type with qualifiers parameter. (arm_init_crypto_builtins): Likewise. gcc/testsuite/ChangeLog: PR target/107674 * gcc.target/arm/mve/mve_vpt.c: New test.
[PATCH 1/3] arm: Fix sign of MVE predicate mve_pred16_t [PR 107674]
Hi,
The ACLE defines mve_pred16_t as an unsigned short. This patch makes
sure GCC treats the predicate as an unsigned type, rather than signed.
Bootstrapped on aarch64-none-eabi and regression tested on arm-none-eabi
and armeb-none-eabi for armv8.1-m.main+mve.fp.
OK for trunk?
gcc/ChangeLog:
PR target/107674
* config/arm/arm-builtins.cc (arm_simd_builtin_type): Rewrite to use
new qualifiers parameter and use unsigned short type for MVE predicate.
(arm_init_builtin): Call arm_simd_builtin_type with qualifiers
parameter.
(arm_init_crypto_builtins): Likewise.
gcc/testsuite/ChangeLog:
PR target/107674
* gcc.target/arm/mve/mve_vpt.c: New test.diff --git a/gcc/config/arm/arm-builtins.cc b/gcc/config/arm/arm-builtins.cc
index
11d7478d9df69139802a9d42c09dd0de7480b60e..6c67cec93ff76a4b42f3a0b305f697142e88fcd9
100644
--- a/gcc/config/arm/arm-builtins.cc
+++ b/gcc/config/arm/arm-builtins.cc
@@ -1489,12 +1489,14 @@ arm_lookup_simd_builtin_type (machine_mode mode,
}
static tree
-arm_simd_builtin_type (machine_mode mode, bool unsigned_p, bool poly_p)
+arm_simd_builtin_type (machine_mode mode, enum arm_type_qualifiers qualifiers)
{
- if (poly_p)
+ if ((qualifiers & qualifier_poly) != 0)
return arm_lookup_simd_builtin_type (mode, qualifier_poly);
- else if (unsigned_p)
+ else if ((qualifiers & qualifier_unsigned) != 0)
return arm_lookup_simd_builtin_type (mode, qualifier_unsigned);
+ else if ((qualifiers & qualifier_predicate) != 0)
+return unsigned_intHI_type_node;
else
return arm_lookup_simd_builtin_type (mode, qualifier_none);
}
@@ -1755,9 +1757,7 @@ arm_init_builtin (unsigned int fcode, arm_builtin_datum
*d,
else
{
eltype
- = arm_simd_builtin_type (op_mode,
-(qualifiers & qualifier_unsigned) != 0,
-(qualifiers & qualifier_poly) != 0);
+ = arm_simd_builtin_type (op_mode, qualifiers);
gcc_assert (eltype != NULL);
/* Add qualifiers. */
@@ -1929,10 +1929,10 @@ static void
arm_init_crypto_builtins (void)
{
tree V16UQI_type_node
-= arm_simd_builtin_type (V16QImode, true, false);
+= arm_simd_builtin_type (V16QImode, qualifier_unsigned);
tree V4USI_type_node
-= arm_simd_builtin_type (V4SImode, true, false);
+= arm_simd_builtin_type (V4SImode, qualifier_unsigned);
tree v16uqi_ftype_v16uqi
= build_function_type_list (V16UQI_type_node, V16UQI_type_node,
diff --git a/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
new file mode 100644
index
..26a565b79dd1348e361b3aa23a1d6e6d13bffce8
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/mve/mve_vpt.c
@@ -0,0 +1,27 @@
+/* { dg-options "-O2" } */
+/* { dg-require-effective-target arm_v8_1m_mve_ok } */
+/* { dg-add-options arm_v8_1m_mve } */
+/* { dg-final { check-function-bodies "**" "" } } */
+#include
+void test0 (uint8_t *a, uint8_t *b, uint8_t *c)
+{
+uint8x16_t va = vldrbq_u8 (a);
+uint8x16_t vb = vldrbq_u8 (b);
+mve_pred16_t p = vcmpeqq_u8 (va, vb);
+uint8x16_t vc = vaddq_x_u8 (va, vb, p);
+vstrbq_p_u8 (c, vc, p);
+}
+/*
+** test0:
+** vldrb.8 q2, \[r0\]
+** vldrb.8 q1, \[r1\]
+** vcmp.i8 eq, q2, q1
+** vmrsr3, p0 @ movhi
+** uxthr3, r3
+** vmsrp0, r3 @ movhi
+** vpst
+** vaddt.i8q3, q2, q1
+** vpst
+** vstrbt.8q3, \[r2\]
+** bx lr
+*/
