* [PATCH] arm: [MVE intrinsics] Fix support for predicate constants [PR target/114801]
@ 2024-04-26 23:10 Christophe Lyon
2024-04-29 13:29 ` Jakub Jelinek
0 siblings, 1 reply; 9+ messages in thread
From: Christophe Lyon @ 2024-04-26 23:10 UTC (permalink / raw)
To: gcc-patches, richard.sandiford, richard.earnshaw, jakub; +Cc: Christophe Lyon
In this PR, we have to handle a case where MVE predicates are supplied
as a const_int, where individual predicates have illegal boolean
values (such as 0xc for a 4-bit boolean predicate). To avoid the ICE,
we canonicalize them, replacing a non-null value with -1.
2024-04-26 Christophe Lyon <christophe.lyon@linaro.org>
Jakub Jelinek <jakub@redhat.com>
PR target/114801
gcc/
* config/arm/arm-mve-builtins.cc
(function_expander::add_input_operand): Handle CONST_INT
predicates.
gcc/testsuite/
* gcc.target/arm/mve/pr114801.c: New test.
---
gcc/config/arm/arm-mve-builtins.cc | 21 +++++++++++-
gcc/testsuite/gcc.target/arm/mve/pr114801.c | 36 +++++++++++++++++++++
2 files changed, 56 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/arm/mve/pr114801.c
diff --git a/gcc/config/arm/arm-mve-builtins.cc b/gcc/config/arm/arm-mve-builtins.cc
index 6a5775c67e5..f338ab36434 100644
--- a/gcc/config/arm/arm-mve-builtins.cc
+++ b/gcc/config/arm/arm-mve-builtins.cc
@@ -43,6 +43,7 @@
#include "stringpool.h"
#include "attribs.h"
#include "diagnostic.h"
+#include "rtx-vector-builder.h"
#include "arm-protos.h"
#include "arm-builtins.h"
#include "arm-mve-builtins.h"
@@ -2205,7 +2206,25 @@ function_expander::add_input_operand (insn_code icode, rtx x)
mode = GET_MODE (x);
}
else if (VALID_MVE_PRED_MODE (mode))
- x = gen_lowpart (mode, x);
+ {
+ if (CONST_INT_P (x) && (mode == V8BImode || mode == V4BImode))
+ {
+ /* In V8BI or V4BI each element has 2 or 4 bits, if those
+ bits aren't all the same, it is UB and gen_lowpart might
+ ICE. Canonicalize all the 2 or 4 bits to all ones if any
+ of them is non-zero. */
+ unsigned HOST_WIDE_INT xi = UINTVAL (x);
+ xi |= ((xi & 0x5555) << 1) | ((xi & 0xaaaa) >> 1);
+ if (mode == V4BImode)
+ xi |= ((xi & 0x3333) << 2) | ((xi & 0xcccc) >> 2);
+ x = gen_int_mode (xi, HImode);
+ }
+ else if (SUBREG_P (x))
+ /* gen_lowpart on a SUBREG can ICE. */
+ x = force_reg (GET_MODE (x), x);
+
+ x = gen_lowpart (mode, x);
+ }
m_ops.safe_grow (m_ops.length () + 1, true);
create_input_operand (&m_ops.last (), x, mode);
diff --git a/gcc/testsuite/gcc.target/arm/mve/pr114801.c b/gcc/testsuite/gcc.target/arm/mve/pr114801.c
new file mode 100644
index 00000000000..676b109f9b8
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/mve/pr114801.c
@@ -0,0 +1,36 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_v8_1m_mve_ok } */
+/* { dg-add-options arm_v8_1m_mve } */
+/* { dg-final { check-function-bodies "**" "" "" } } */
+
+#include <arm_mve.h>
+
+/*
+** test_32:
+**...
+** mov r[0-9]+, #65535 @ movhi
+**...
+*/
+uint32x4_t test_32() {
+ return vdupq_m_n_u32(vdupq_n_u32(0), 0, 0xcccc);
+}
+
+/*
+** test_16:
+**...
+** mov r[0-9]+, #52428 @ movhi
+**...
+*/
+uint16x8_t test_16() {
+ return vdupq_m_n_u16(vdupq_n_u16(0), 0, 0xcccc);
+}
+
+/*
+** test_8:
+**...
+** mov r[0-9]+, #52428 @ movhi
+**...
+*/
+uint8x16_t test_8() {
+ return vdupq_m_n_u8(vdupq_n_u8(0), 0, 0xcccc);
+}
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] arm: [MVE intrinsics] Fix support for predicate constants [PR target/114801]
2024-04-26 23:10 [PATCH] arm: [MVE intrinsics] Fix support for predicate constants [PR target/114801] Christophe Lyon
@ 2024-04-29 13:29 ` Jakub Jelinek
2024-04-29 13:43 ` Christophe Lyon
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2024-04-29 13:29 UTC (permalink / raw)
To: Christophe Lyon; +Cc: gcc-patches, richard.sandiford, richard.earnshaw
On Fri, Apr 26, 2024 at 11:10:12PM +0000, Christophe Lyon wrote:
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/mve/pr114801.c
> @@ -0,0 +1,36 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target arm_v8_1m_mve_ok } */
> +/* { dg-add-options arm_v8_1m_mve } */
> +/* { dg-final { check-function-bodies "**" "" "" } } */
> +
> +#include <arm_mve.h>
> +
> +/*
> +** test_32:
> +**...
> +** mov r[0-9]+, #65535 @ movhi
> +**...
> +*/
> +uint32x4_t test_32() {
> + return vdupq_m_n_u32(vdupq_n_u32(0), 0, 0xcccc);
Just a testcase nit. I think testing 0xcccc isn't that useful,
it tests the same 4 bits 4 times.
Might be more interesting to test 4 different 4 bit elements,
one of them 0 (to verify it doesn't turn that into all ones),
one all 1s (that is the other valid case) and then 2 random
other values in between.
> +}
> +
> +/*
> +** test_16:
> +**...
> +** mov r[0-9]+, #52428 @ movhi
> +**...
> +*/
> +uint16x8_t test_16() {
> + return vdupq_m_n_u16(vdupq_n_u16(0), 0, 0xcccc);
And for these it can actually test all 4 possible 2 bit elements,
so say 0x3021
> +}
> +
> +/*
> +** test_8:
> +**...
> +** mov r[0-9]+, #52428 @ movhi
> +**...
> +*/
> +uint8x16_t test_8() {
> + return vdupq_m_n_u8(vdupq_n_u8(0), 0, 0xcccc);
and here use some random pattern.
BTW, the patch is ok for 14.1 if it is approved and committed today
(so that it can be cherry-picked tomorrow morning at latest to the branch).
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] arm: [MVE intrinsics] Fix support for predicate constants [PR target/114801]
2024-04-29 13:29 ` Jakub Jelinek
@ 2024-04-29 13:43 ` Christophe Lyon
2024-05-07 16:19 ` [PATCH v2] " Christophe Lyon
0 siblings, 1 reply; 9+ messages in thread
From: Christophe Lyon @ 2024-04-29 13:43 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches, richard.sandiford, richard.earnshaw
On Mon, 29 Apr 2024 at 15:29, Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Fri, Apr 26, 2024 at 11:10:12PM +0000, Christophe Lyon wrote:
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/arm/mve/pr114801.c
> > @@ -0,0 +1,36 @@
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target arm_v8_1m_mve_ok } */
> > +/* { dg-add-options arm_v8_1m_mve } */
> > +/* { dg-final { check-function-bodies "**" "" "" } } */
> > +
> > +#include <arm_mve.h>
> > +
> > +/*
> > +** test_32:
> > +**...
> > +** mov r[0-9]+, #65535 @ movhi
> > +**...
> > +*/
> > +uint32x4_t test_32() {
> > + return vdupq_m_n_u32(vdupq_n_u32(0), 0, 0xcccc);
>
> Just a testcase nit. I think testing 0xcccc isn't that useful,
> it tests the same 4 bits 4 times.
> Might be more interesting to test 4 different 4 bit elements,
> one of them 0 (to verify it doesn't turn that into all ones),
> one all 1s (that is the other valid case) and then 2 random
> other values in between.
>
> > +}
> > +
> > +/*
> > +** test_16:
> > +**...
> > +** mov r[0-9]+, #52428 @ movhi
> > +**...
> > +*/
> > +uint16x8_t test_16() {
> > + return vdupq_m_n_u16(vdupq_n_u16(0), 0, 0xcccc);
>
> And for these it can actually test all 4 possible 2 bit elements,
> so say 0x3021
>
> > +}
> > +
> > +/*
> > +** test_8:
> > +**...
> > +** mov r[0-9]+, #52428 @ movhi
> > +**...
> > +*/
> > +uint8x16_t test_8() {
> > + return vdupq_m_n_u8(vdupq_n_u8(0), 0, 0xcccc);
>
> and here use some random pattern.
>
> BTW, the patch is ok for 14.1 if it is approved and committed today
> (so that it can be cherry-picked tomorrow morning at latest to the branch).
Thanks for your comments, I'll update the testcase, but Andre provided
additional info in the PR:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114801#c17
I tried just removing the call to gcc_unreachable in
rtx_vector_builder::find_cached_value and that does the trick, but I'm
worried by such a change.
Christophe
>
> Jakub
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] arm: [MVE intrinsics] Fix support for predicate constants [PR target/114801]
2024-04-29 13:43 ` Christophe Lyon
@ 2024-05-07 16:19 ` Christophe Lyon
2024-10-18 12:57 ` Andre Vieira (lists)
0 siblings, 1 reply; 9+ messages in thread
From: Christophe Lyon @ 2024-05-07 16:19 UTC (permalink / raw)
To: gcc-patches, richard.sandiford, richard.earnshaw, jakub,
andre.simoesdiasvieira
Cc: Christophe Lyon
In this PR, we have to handle a case where MVE predicates are supplied
as a const_int, where individual predicates have illegal boolean
values (such as 0xc for a 4-bit boolean predicate). To avoid the ICE,
we hide the constant behind an unspec.
On MVE V8BI and V4BI multi-bit masks are interpreted byte-by-byte at
instruction level, see
https://developer.arm.com/documentation/101028/0012/14--M-profile-Vector-Extension--MVE--intrinsics.
This is a workaround until we change such predicates representation to
V16BImode.
2024-05-06 Christophe Lyon <christophe.lyon@linaro.org>
Jakub Jelinek <jakub@redhat.com>
PR target/114801
gcc/
* config/arm/arm-mve-builtins.cc
(function_expander::add_input_operand): Handle CONST_INT
predicates.
* mve.md (set_mve_const_pred): New pattern.
* unspec.md (MVE_PRED): New unspec.
gcc/testsuite/
* gcc.target/arm/mve/pr114801.c: New test.
---
gcc/config/arm/arm-mve-builtins.cc | 27 ++++++++++++++-
gcc/config/arm/mve.md | 12 +++++++
gcc/config/arm/unspecs.md | 1 +
gcc/testsuite/gcc.target/arm/mve/pr114801.c | 37 +++++++++++++++++++++
4 files changed, 76 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/arm/mve/pr114801.c
diff --git a/gcc/config/arm/arm-mve-builtins.cc b/gcc/config/arm/arm-mve-builtins.cc
index 6a5775c67e5..7d5af649857 100644
--- a/gcc/config/arm/arm-mve-builtins.cc
+++ b/gcc/config/arm/arm-mve-builtins.cc
@@ -2205,7 +2205,32 @@ function_expander::add_input_operand (insn_code icode, rtx x)
mode = GET_MODE (x);
}
else if (VALID_MVE_PRED_MODE (mode))
- x = gen_lowpart (mode, x);
+ {
+ if (CONST_INT_P (x) && (mode == V8BImode || mode == V4BImode))
+ {
+ /* In V8BI or V4BI each element has 2 or 4 bits, if those
+ bits aren't all the same, gen_lowpart might ICE. Hide
+ the move behind an unspec to avoid this.
+ V8BI and V4BI multi-bit masks are interpreted
+ byte-by-byte at instruction level, see
+ https://developer.arm.com/documentation/101028/0012/14--M-profile-Vector-Extension--MVE--intrinsics. */
+ unsigned HOST_WIDE_INT xi = UINTVAL (x);
+ if ((xi & 0x5555) != ((xi >> 1) & 0x5555)
+ || (mode == V4BImode
+ && (xi & 0x3333) != ((xi >> 2) & 0x3333)))
+ {
+ rtx unspec_x;
+ unspec_x = gen_rtx_UNSPEC (HImode, gen_rtvec (1, x), MVE_PRED);
+ x = force_reg (HImode, unspec_x);
+ }
+
+ }
+ else if (SUBREG_P (x))
+ /* gen_lowpart on a SUBREG can ICE. */
+ x = force_reg (GET_MODE (x), x);
+
+ x = gen_lowpart (mode, x);
+ }
m_ops.safe_grow (m_ops.length () + 1, true);
create_input_operand (&m_ops.last (), x, mode);
diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md
index 35916f62604..d337422d695 100644
--- a/gcc/config/arm/mve.md
+++ b/gcc/config/arm/mve.md
@@ -6621,3 +6621,15 @@ (define_expand "@arm_mve_reinterpret<mode>"
}
}
)
+
+;; Hide predicate constants from optimizers
+(define_insn "set_mve_const_pred"
+ [(set
+ (match_operand:HI 0 "s_register_operand" "=r")
+ (unspec:HI [(match_operand:HI 1 "general_operand" "n")] MVE_PRED))]
+ "TARGET_HAVE_MVE"
+{
+ return "movw%?\t%0, %L1\t%@ set_mve_const_pred";
+}
+ [(set_attr "type" "mov_imm")]
+)
diff --git a/gcc/config/arm/unspecs.md b/gcc/config/arm/unspecs.md
index 4713ec840ab..336f2fe08e6 100644
--- a/gcc/config/arm/unspecs.md
+++ b/gcc/config/arm/unspecs.md
@@ -1256,4 +1256,5 @@ (define_c_enum "unspec" [
SQRSHRL_48
VSHLCQ_M_
REINTERPRET
+ MVE_PRED
])
diff --git a/gcc/testsuite/gcc.target/arm/mve/pr114801.c b/gcc/testsuite/gcc.target/arm/mve/pr114801.c
new file mode 100644
index 00000000000..fb3e4d855f9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/mve/pr114801.c
@@ -0,0 +1,37 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target arm_v8_1m_mve_ok } */
+/* { dg-options "-O2" } */
+/* { dg-add-options arm_v8_1m_mve } */
+/* { dg-final { check-function-bodies "**" "" "" } } */
+
+#include <arm_mve.h>
+
+/*
+** test_32:
+**...
+** movw r[0-9]+, 52428 @ set_mve_const_pred
+**...
+*/
+uint32x4_t test_32() {
+ return vdupq_m_n_u32(vdupq_n_u32(0xffffffff), 0, 0xcccc);
+}
+
+/*
+** test_16:
+**...
+** movw r[0-9]+, 6927 @ set_mve_const_pred
+**...
+*/
+uint16x8_t test_16() {
+ return vdupq_m_n_u16(vdupq_n_u16(0xffff), 0, 0x1b0f);
+}
+
+/*
+** test_8:
+**...
+** mov r[0-9]+, #23055 @ movhi
+**...
+*/
+uint8x16_t test_8() {
+ return vdupq_m_n_u8(vdupq_n_u8(0xff), 0, 0x5a0f);
+}
--
2.34.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] arm: [MVE intrinsics] Fix support for predicate constants [PR target/114801]
2024-05-07 16:19 ` [PATCH v2] " Christophe Lyon
@ 2024-10-18 12:57 ` Andre Vieira (lists)
2024-11-01 11:12 ` Richard Earnshaw (lists)
0 siblings, 1 reply; 9+ messages in thread
From: Andre Vieira (lists) @ 2024-10-18 12:57 UTC (permalink / raw)
To: Christophe Lyon, gcc-patches, richard.sandiford, richard.earnshaw, jakub
Hi,
This looks like an acceptable work around. We special case behavior that
I'm not sure we can express in ways GCC can understand or will make use
of, whilst at the same time we keep expressing behavior it does
understand and can optimize.
Nice idea!
LGTM, needs maintainer approval though.
Kind regards,
Andre
On 07/05/2024 17:19, Christophe Lyon wrote:
> In this PR, we have to handle a case where MVE predicates are supplied
> as a const_int, where individual predicates have illegal boolean
> values (such as 0xc for a 4-bit boolean predicate). To avoid the ICE,
> we hide the constant behind an unspec.
>
> On MVE V8BI and V4BI multi-bit masks are interpreted byte-by-byte at
> instruction level, see
> https://developer.arm.com/documentation/101028/0012/14--M-profile-Vector-Extension--MVE--intrinsics.
>
> This is a workaround until we change such predicates representation to
> V16BImode.
>
> 2024-05-06 Christophe Lyon <christophe.lyon@linaro.org>
> Jakub Jelinek <jakub@redhat.com>
>
> PR target/114801
> gcc/
> * config/arm/arm-mve-builtins.cc
> (function_expander::add_input_operand): Handle CONST_INT
> predicates.
> * mve.md (set_mve_const_pred): New pattern.
> * unspec.md (MVE_PRED): New unspec.
>
> gcc/testsuite/
> * gcc.target/arm/mve/pr114801.c: New test.
> ---
> gcc/config/arm/arm-mve-builtins.cc | 27 ++++++++++++++-
> gcc/config/arm/mve.md | 12 +++++++
> gcc/config/arm/unspecs.md | 1 +
> gcc/testsuite/gcc.target/arm/mve/pr114801.c | 37 +++++++++++++++++++++
> 4 files changed, 76 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/arm/mve/pr114801.c
>
> diff --git a/gcc/config/arm/arm-mve-builtins.cc b/gcc/config/arm/arm-mve-builtins.cc
> index 6a5775c67e5..7d5af649857 100644
> --- a/gcc/config/arm/arm-mve-builtins.cc
> +++ b/gcc/config/arm/arm-mve-builtins.cc
> @@ -2205,7 +2205,32 @@ function_expander::add_input_operand (insn_code icode, rtx x)
> mode = GET_MODE (x);
> }
> else if (VALID_MVE_PRED_MODE (mode))
> - x = gen_lowpart (mode, x);
> + {
> + if (CONST_INT_P (x) && (mode == V8BImode || mode == V4BImode))
> + {
> + /* In V8BI or V4BI each element has 2 or 4 bits, if those
> + bits aren't all the same, gen_lowpart might ICE. Hide
> + the move behind an unspec to avoid this.
> + V8BI and V4BI multi-bit masks are interpreted
> + byte-by-byte at instruction level, see
> + https://developer.arm.com/documentation/101028/0012/14--M-profile-Vector-Extension--MVE--intrinsics. */
> + unsigned HOST_WIDE_INT xi = UINTVAL (x);
> + if ((xi & 0x5555) != ((xi >> 1) & 0x5555)
> + || (mode == V4BImode
> + && (xi & 0x3333) != ((xi >> 2) & 0x3333)))
> + {
> + rtx unspec_x;
> + unspec_x = gen_rtx_UNSPEC (HImode, gen_rtvec (1, x), MVE_PRED);
> + x = force_reg (HImode, unspec_x);
> + }
> +
> + }
> + else if (SUBREG_P (x))
> + /* gen_lowpart on a SUBREG can ICE. */
> + x = force_reg (GET_MODE (x), x);
> +
> + x = gen_lowpart (mode, x);
> + }
>
> m_ops.safe_grow (m_ops.length () + 1, true);
> create_input_operand (&m_ops.last (), x, mode);
> diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md
> index 35916f62604..d337422d695 100644
> --- a/gcc/config/arm/mve.md
> +++ b/gcc/config/arm/mve.md
> @@ -6621,3 +6621,15 @@ (define_expand "@arm_mve_reinterpret<mode>"
> }
> }
> )
> +
> +;; Hide predicate constants from optimizers
> +(define_insn "set_mve_const_pred"
> + [(set
> + (match_operand:HI 0 "s_register_operand" "=r")
> + (unspec:HI [(match_operand:HI 1 "general_operand" "n")] MVE_PRED))]
> + "TARGET_HAVE_MVE"
> +{
> + return "movw%?\t%0, %L1\t%@ set_mve_const_pred";
> +}
> + [(set_attr "type" "mov_imm")]
> +)
> diff --git a/gcc/config/arm/unspecs.md b/gcc/config/arm/unspecs.md
> index 4713ec840ab..336f2fe08e6 100644
> --- a/gcc/config/arm/unspecs.md
> +++ b/gcc/config/arm/unspecs.md
> @@ -1256,4 +1256,5 @@ (define_c_enum "unspec" [
> SQRSHRL_48
> VSHLCQ_M_
> REINTERPRET
> + MVE_PRED
> ])
> diff --git a/gcc/testsuite/gcc.target/arm/mve/pr114801.c b/gcc/testsuite/gcc.target/arm/mve/pr114801.c
> new file mode 100644
> index 00000000000..fb3e4d855f9
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/mve/pr114801.c
> @@ -0,0 +1,37 @@
> +/* { dg-do compile } */
> +/* { dg-require-effective-target arm_v8_1m_mve_ok } */
> +/* { dg-options "-O2" } */
> +/* { dg-add-options arm_v8_1m_mve } */
> +/* { dg-final { check-function-bodies "**" "" "" } } */
> +
> +#include <arm_mve.h>
> +
> +/*
> +** test_32:
> +**...
> +** movw r[0-9]+, 52428 @ set_mve_const_pred
> +**...
> +*/
> +uint32x4_t test_32() {
> + return vdupq_m_n_u32(vdupq_n_u32(0xffffffff), 0, 0xcccc);
> +}
> +
> +/*
> +** test_16:
> +**...
> +** movw r[0-9]+, 6927 @ set_mve_const_pred
> +**...
> +*/
> +uint16x8_t test_16() {
> + return vdupq_m_n_u16(vdupq_n_u16(0xffff), 0, 0x1b0f);
> +}
> +
> +/*
> +** test_8:
> +**...
> +** mov r[0-9]+, #23055 @ movhi
> +**...
> +*/
> +uint8x16_t test_8() {
> + return vdupq_m_n_u8(vdupq_n_u8(0xff), 0, 0x5a0f);
> +}
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] arm: [MVE intrinsics] Fix support for predicate constants [PR target/114801]
2024-10-18 12:57 ` Andre Vieira (lists)
@ 2024-11-01 11:12 ` Richard Earnshaw (lists)
2024-11-01 11:22 ` Jakub Jelinek
0 siblings, 1 reply; 9+ messages in thread
From: Richard Earnshaw (lists) @ 2024-11-01 11:12 UTC (permalink / raw)
To: Andre Vieira (lists),
Christophe Lyon, gcc-patches, richard.sandiford, jakub
On 18/10/2024 13:57, Andre Vieira (lists) wrote:
> Hi,
>
> This looks like an acceptable work around. We special case behavior that I'm not sure we can express in ways GCC can understand or will make use of, whilst at the same time we keep expressing behavior it does understand and can optimize.
>
> Nice idea!
>
> LGTM, needs maintainer approval though.
>
As Jakub said in the PR, this is really just papering over a general problem in the compiler. As such, it's OK for the gcc-14 branch, if we really need it, but not really suitable for trunk.
I think the problem is related to failing to correclty canonicalize non-standard bool types in trunk_int_for_mode. The code there has:
/* Canonicalize BImode to 0 and STORE_FLAG_VALUE. */
if (smode == BImode)
return c & 1 ? STORE_FLAG_VALUE : 0;
and that means we fail to generate canonical boolean values, so later on the compiler asserts when trying to pick non-standard values out.
Perhaps instead we need something like:
/* Canonicalize scalar boolean modes to 0 and STORE_FLAG_VALUE. */
if (smode >= MIN_MODE_BOOL && smode <= MAX_MODE_BOOL)
return c & 1 ? STORE_FLAG_VALUE : 0;
Jakub?
R.
> Kind regards,
> Andre
>
> On 07/05/2024 17:19, Christophe Lyon wrote:
>> In this PR, we have to handle a case where MVE predicates are supplied
>> as a const_int, where individual predicates have illegal boolean
>> values (such as 0xc for a 4-bit boolean predicate). To avoid the ICE,
>> we hide the constant behind an unspec.
>>
>> On MVE V8BI and V4BI multi-bit masks are interpreted byte-by-byte at
>> instruction level, see
>> https://developer.arm.com/documentation/101028/0012/14--M-profile-Vector-Extension--MVE--intrinsics.
>>
>> This is a workaround until we change such predicates representation to
>> V16BImode.
>>
>> 2024-05-06 Christophe Lyon <christophe.lyon@linaro.org>
>> Jakub Jelinek <jakub@redhat.com>
>>
>> PR target/114801
>> gcc/
>> * config/arm/arm-mve-builtins.cc
>> (function_expander::add_input_operand): Handle CONST_INT
>> predicates.
>> * mve.md (set_mve_const_pred): New pattern.
>> * unspec.md (MVE_PRED): New unspec.
>>
>> gcc/testsuite/
>> * gcc.target/arm/mve/pr114801.c: New test.
>> ---
>> gcc/config/arm/arm-mve-builtins.cc | 27 ++++++++++++++-
>> gcc/config/arm/mve.md | 12 +++++++
>> gcc/config/arm/unspecs.md | 1 +
>> gcc/testsuite/gcc.target/arm/mve/pr114801.c | 37 +++++++++++++++++++++
>> 4 files changed, 76 insertions(+), 1 deletion(-)
>> create mode 100644 gcc/testsuite/gcc.target/arm/mve/pr114801.c
>>
>> diff --git a/gcc/config/arm/arm-mve-builtins.cc b/gcc/config/arm/arm-mve-builtins.cc
>> index 6a5775c67e5..7d5af649857 100644
>> --- a/gcc/config/arm/arm-mve-builtins.cc
>> +++ b/gcc/config/arm/arm-mve-builtins.cc
>> @@ -2205,7 +2205,32 @@ function_expander::add_input_operand (insn_code icode, rtx x)
>> mode = GET_MODE (x);
>> }
>> else if (VALID_MVE_PRED_MODE (mode))
>> - x = gen_lowpart (mode, x);
>> + {
>> + if (CONST_INT_P (x) && (mode == V8BImode || mode == V4BImode))
>> + {
>> + /* In V8BI or V4BI each element has 2 or 4 bits, if those
>> + bits aren't all the same, gen_lowpart might ICE. Hide
>> + the move behind an unspec to avoid this.
>> + V8BI and V4BI multi-bit masks are interpreted
>> + byte-by-byte at instruction level, see
>> + https://developer.arm.com/documentation/101028/0012/14--M-profile-Vector-Extension--MVE--intrinsics. */
>> + unsigned HOST_WIDE_INT xi = UINTVAL (x);
>> + if ((xi & 0x5555) != ((xi >> 1) & 0x5555)
>> + || (mode == V4BImode
>> + && (xi & 0x3333) != ((xi >> 2) & 0x3333)))
>> + {
>> + rtx unspec_x;
>> + unspec_x = gen_rtx_UNSPEC (HImode, gen_rtvec (1, x), MVE_PRED);
>> + x = force_reg (HImode, unspec_x);
>> + }
>> +
>> + }
>> + else if (SUBREG_P (x))
>> + /* gen_lowpart on a SUBREG can ICE. */
>> + x = force_reg (GET_MODE (x), x);
>> +
>> + x = gen_lowpart (mode, x);
>> + }
>> m_ops.safe_grow (m_ops.length () + 1, true);
>> create_input_operand (&m_ops.last (), x, mode);
>> diff --git a/gcc/config/arm/mve.md b/gcc/config/arm/mve.md
>> index 35916f62604..d337422d695 100644
>> --- a/gcc/config/arm/mve.md
>> +++ b/gcc/config/arm/mve.md
>> @@ -6621,3 +6621,15 @@ (define_expand "@arm_mve_reinterpret<mode>"
>> }
>> }
>> )
>> +
>> +;; Hide predicate constants from optimizers
>> +(define_insn "set_mve_const_pred"
>> + [(set
>> + (match_operand:HI 0 "s_register_operand" "=r")
>> + (unspec:HI [(match_operand:HI 1 "general_operand" "n")] MVE_PRED))]
>> + "TARGET_HAVE_MVE"
>> +{
>> + return "movw%?\t%0, %L1\t%@ set_mve_const_pred";
>> +}
>> + [(set_attr "type" "mov_imm")]
>> +)
>> diff --git a/gcc/config/arm/unspecs.md b/gcc/config/arm/unspecs.md
>> index 4713ec840ab..336f2fe08e6 100644
>> --- a/gcc/config/arm/unspecs.md
>> +++ b/gcc/config/arm/unspecs.md
>> @@ -1256,4 +1256,5 @@ (define_c_enum "unspec" [
>> SQRSHRL_48
>> VSHLCQ_M_
>> REINTERPRET
>> + MVE_PRED
>> ])
>> diff --git a/gcc/testsuite/gcc.target/arm/mve/pr114801.c b/gcc/testsuite/gcc.target/arm/mve/pr114801.c
>> new file mode 100644
>> index 00000000000..fb3e4d855f9
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/mve/pr114801.c
>> @@ -0,0 +1,37 @@
>> +/* { dg-do compile } */
>> +/* { dg-require-effective-target arm_v8_1m_mve_ok } */
>> +/* { dg-options "-O2" } */
>> +/* { dg-add-options arm_v8_1m_mve } */
>> +/* { dg-final { check-function-bodies "**" "" "" } } */
>> +
>> +#include <arm_mve.h>
>> +
>> +/*
>> +** test_32:
>> +**...
>> +** movw r[0-9]+, 52428 @ set_mve_const_pred
>> +**...
>> +*/
>> +uint32x4_t test_32() {
>> + return vdupq_m_n_u32(vdupq_n_u32(0xffffffff), 0, 0xcccc);
>> +}
>> +
>> +/*
>> +** test_16:
>> +**...
>> +** movw r[0-9]+, 6927 @ set_mve_const_pred
>> +**...
>> +*/
>> +uint16x8_t test_16() {
>> + return vdupq_m_n_u16(vdupq_n_u16(0xffff), 0, 0x1b0f);
>> +}
>> +
>> +/*
>> +** test_8:
>> +**...
>> +** mov r[0-9]+, #23055 @ movhi
>> +**...
>> +*/
>> +uint8x16_t test_8() {
>> + return vdupq_m_n_u8(vdupq_n_u8(0xff), 0, 0x5a0f);
>> +}
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] arm: [MVE intrinsics] Fix support for predicate constants [PR target/114801]
2024-11-01 11:12 ` Richard Earnshaw (lists)
@ 2024-11-01 11:22 ` Jakub Jelinek
2024-11-01 11:24 ` Richard Earnshaw (lists)
0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2024-11-01 11:22 UTC (permalink / raw)
To: Richard Earnshaw (lists)
Cc: Andre Vieira (lists), Christophe Lyon, gcc-patches, richard.sandiford
On Fri, Nov 01, 2024 at 11:12:29AM +0000, Richard Earnshaw (lists) wrote:
> As Jakub said in the PR, this is really just papering over a general problem in the compiler. As such, it's OK for the gcc-14 branch, if we really need it, but not really suitable for trunk.
>
> I think the problem is related to failing to correclty canonicalize non-standard bool types in trunk_int_for_mode. The code there has:
>
> /* Canonicalize BImode to 0 and STORE_FLAG_VALUE. */
> if (smode == BImode)
> return c & 1 ? STORE_FLAG_VALUE : 0;
>
> and that means we fail to generate canonical boolean values, so later on the compiler asserts when trying to pick non-standard values out.
>
> Perhaps instead we need something like:
>
> /* Canonicalize scalar boolean modes to 0 and STORE_FLAG_VALUE. */
> if (smode >= MIN_MODE_BOOL && smode <= MAX_MODE_BOOL)
> return c & 1 ? STORE_FLAG_VALUE : 0;
>
> Jakub?
A mode which has 4 or 16 possible valid values, not just 2, shouldn't be
considered a boolean mode, it is a partial int.
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] arm: [MVE intrinsics] Fix support for predicate constants [PR target/114801]
2024-11-01 11:22 ` Jakub Jelinek
@ 2024-11-01 11:24 ` Richard Earnshaw (lists)
2024-11-01 14:29 ` Richard Earnshaw
0 siblings, 1 reply; 9+ messages in thread
From: Richard Earnshaw (lists) @ 2024-11-01 11:24 UTC (permalink / raw)
To: Jakub Jelinek
Cc: Andre Vieira (lists), Christophe Lyon, gcc-patches, richard.sandiford
On 01/11/2024 11:22, Jakub Jelinek wrote:
> On Fri, Nov 01, 2024 at 11:12:29AM +0000, Richard Earnshaw (lists) wrote:
>> As Jakub said in the PR, this is really just papering over a general problem in the compiler. As such, it's OK for the gcc-14 branch, if we really need it, but not really suitable for trunk.
>>
>> I think the problem is related to failing to correclty canonicalize non-standard bool types in trunk_int_for_mode. The code there has:
>>
>> /* Canonicalize BImode to 0 and STORE_FLAG_VALUE. */
>> if (smode == BImode)
>> return c & 1 ? STORE_FLAG_VALUE : 0;
>>
>> and that means we fail to generate canonical boolean values, so later on the compiler asserts when trying to pick non-standard values out.
>>
>> Perhaps instead we need something like:
>>
>> /* Canonicalize scalar boolean modes to 0 and STORE_FLAG_VALUE. */
>> if (smode >= MIN_MODE_BOOL && smode <= MAX_MODE_BOOL)
>> return c & 1 ? STORE_FLAG_VALUE : 0;
>>
>> Jakub?
>
> A mode which has 4 or 16 possible valid values, not just 2, shouldn't be
> considered a boolean mode, it is a partial int.
>
> Jakub
>
Its a bool in just the same way that a bool is stored in a char. In this case it's packed into 2 or 4 bits when put in a vector.
R.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] arm: [MVE intrinsics] Fix support for predicate constants [PR target/114801]
2024-11-01 11:24 ` Richard Earnshaw (lists)
@ 2024-11-01 14:29 ` Richard Earnshaw
0 siblings, 0 replies; 9+ messages in thread
From: Richard Earnshaw @ 2024-11-01 14:29 UTC (permalink / raw)
To: Jakub Jelinek
Cc: Andre Vieira (lists), Christophe Lyon, gcc-patches, richard.sandiford
On 01/11/2024 11:24, Richard Earnshaw (lists) wrote:
> On 01/11/2024 11:22, Jakub Jelinek wrote:
>> On Fri, Nov 01, 2024 at 11:12:29AM +0000, Richard Earnshaw (lists) wrote:
>>> As Jakub said in the PR, this is really just papering over a general problem in the compiler. As such, it's OK for the gcc-14 branch, if we really need it, but not really suitable for trunk.
>>>
>>> I think the problem is related to failing to correclty canonicalize non-standard bool types in trunk_int_for_mode. The code there has:
>>>
>>> /* Canonicalize BImode to 0 and STORE_FLAG_VALUE. */
>>> if (smode == BImode)
>>> return c & 1 ? STORE_FLAG_VALUE : 0;
>>>
>>> and that means we fail to generate canonical boolean values, so later on the compiler asserts when trying to pick non-standard values out.
>>>
>>> Perhaps instead we need something like:
>>>
>>> /* Canonicalize scalar boolean modes to 0 and STORE_FLAG_VALUE. */
>>> if (smode >= MIN_MODE_BOOL && smode <= MAX_MODE_BOOL)
>>> return c & 1 ? STORE_FLAG_VALUE : 0;
>>>
>>> Jakub?
>>
>> A mode which has 4 or 16 possible valid values, not just 2, shouldn't be
>> considered a boolean mode, it is a partial int.
>>
>> Jakub
>>
>
> Its a bool in just the same way that a bool is stored in a char. In this case it's packed into 2 or 4 bits when put in a vector.
>
> R.
Actually, no it's not that simple. I need to go think about this some more...
R.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-11-01 14:29 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-26 23:10 [PATCH] arm: [MVE intrinsics] Fix support for predicate constants [PR target/114801] Christophe Lyon
2024-04-29 13:29 ` Jakub Jelinek
2024-04-29 13:43 ` Christophe Lyon
2024-05-07 16:19 ` [PATCH v2] " Christophe Lyon
2024-10-18 12:57 ` Andre Vieira (lists)
2024-11-01 11:12 ` Richard Earnshaw (lists)
2024-11-01 11:22 ` Jakub Jelinek
2024-11-01 11:24 ` Richard Earnshaw (lists)
2024-11-01 14:29 ` Richard Earnshaw
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).