RE: [PATCH] aarch64: PR target/102252 Invalid addressing mode for SVE load predicate

[Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>]

[-- Attachment #1: Type: text/plain, Size: 4970 bytes --]

Hi Richard,

> -----Original Message-----
> From: Richard Sandiford <richard.sandiford@arm.com>
> Sent: 13 September 2021 12:09
> To: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] aarch64: PR target/102252 Invalid addressing mode for
> SVE load predicate
> 
> Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> writes:
> > Hi all,
> >
> > In the testcase we generate invalid assembly for an SVE load predicate
> instruction.
> > The RTL for the insn is:
> > (insn 9 8 10 (set (reg:VNx16BI 68 p0)
> >         (mem:VNx16BI (plus:DI (mult:DI (reg:DI 1 x1 [93])
> >                     (const_int 8 [0x8]))
> >                 (reg/f:DI 0 x0 [92])) [2 work_3(D)->array[offset_4(D)]+0 S8 A16]))
> >
> > That addressing mode is not valid for the instruction [1] as it only accepts
> the addressing mode:
> > [<Xn|SP>{, #<imm>, MUL VL}]
> >
> > This patch rejects the register index form for SVE predicate modes.
> >
> > Bootstrapped and tested on aarch64-none-linux-gnu.
> >
> > Ok for trunk?
> > Thanks,
> > Kyrill
> >
> > [1] https://developer.arm.com/documentation/ddi0602/2021-06/SVE-
> Instructions/LDR--predicate---Load-predicate-register-
> >
> > gcc/ChangeLog:
> >
> >         PR target/102252
> >         * config/aarch64/aarch64.c (aarch64_classify_address): Don't allow
> >         register index for SVE predicate modes.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         PR target/102252
> >         * g++.target/aarch64/sve/pr102252.C: New test.
> >
> > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> > index
> e37922db0007e3b4b559cda65f135247f4fb1b9f..e6253edeb55cdcc3dbc7303
> e03bad26dd519c4b1 100644
> > --- a/gcc/config/aarch64/aarch64.c
> > +++ b/gcc/config/aarch64/aarch64.c
> > @@ -9770,7 +9770,7 @@ aarch64_classify_address (struct
> aarch64_address_info *info,
> >  			    || mode == TImode
> >  			    || mode == TFmode
> >  			    || (BYTES_BIG_ENDIAN && advsimd_struct_p));
> > -
> > +  bool sve_pred_p = (vec_flags & VEC_SVE_PRED) != 0;
> >    /* If we are dealing with ADDR_QUERY_LDP_STP_N that means the
> incoming mode
> >       corresponds to the actual size of the memory being loaded/stored and
> the
> >       mode of the corresponding addressing mode is half of that.  */
> > @@ -9779,12 +9779,14 @@ aarch64_classify_address (struct
> aarch64_address_info *info,
> >      mode = DFmode;
> >
> >    bool allow_reg_index_p = (!load_store_pair_p
> > +			    && !sve_pred_p
> >  			    && (known_lt (GET_MODE_SIZE (mode), 16)
> >  				|| vec_flags == VEC_ADVSIMD
> >  				|| vec_flags & VEC_SVE_DATA));
> 
> I think the known_lt (GET_MODE_SIZE (mode), 16) is really there for
> non-vector cases, with the ||s enumerating the valid vector cases.
> So how about:
> 
>   bool allow_reg_index_p = (!load_store_pair_p
>  			    && ((vec_flags == 0
> 				 && known_lt (GET_MODE_SIZE (mode), 16))
>  				|| vec_flags == VEC_ADVSIMD
>  				|| vec_flags & VEC_SVE_DATA));
> 
> instead?  OK with that change from my POV.

Yeah, that works.
Thanks, here's what I've committed. I'll wait a bit before backporting to the branches.

Kyrill

> 
> Thanks,
> Richard
> 
> >
> > -  /* For SVE, only accept [Rn], [Rn, Rm, LSL #shift] and
> > -     [Rn, #offset, MUL VL].  */
> > +  /* For SVE, only accept [Rn], [Rn, #offset, MUL VL] and [Rn, Rm, LSL
> #shift].
> > +     The latter is not valid for SVE predicates, and that's rejected through
> > +     allow_reg_index_p above.  */
> >    if ((vec_flags & (VEC_SVE_DATA | VEC_SVE_PRED)) != 0
> >        && (code != REG && code != PLUS))
> >      return false;
> > diff --git a/gcc/testsuite/g++.target/aarch64/sve/pr102252.C
> b/gcc/testsuite/g++.target/aarch64/sve/pr102252.C
> > new file mode 100644
> > index
> 0000000000000000000000000000000000000000..f90f1218555f0dfdb0253fe
> 83c656ba03b1aac43
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.target/aarch64/sve/pr102252.C
> > @@ -0,0 +1,37 @@
> > +/* PR target/102252.  */
> > +/* { dg-do assemble { target aarch64_asm_sve_ok } } */
> > +/* { dg-options "-march=armv8.2-a+sve -msve-vector-bits=512" } */
> > +
> > +/* We used to generate invalid assembly for SVE predicate loads.  */
> > +
> > +#include <arm_sve.h>
> > +
> > +class SimdBool
> > +{
> > +private:
> > +    typedef svbool_t simdInternalType_
> __attribute__((arm_sve_vector_bits(512)));
> > +
> > +public:
> > +    SimdBool() {}
> > +
> > +    simdInternalType_ simdInternal_;
> > +
> > +};
> > +
> > +static svfloat32_t selectByMask(svfloat32_t a, SimdBool m) {
> > +    return svsel_f32(m.simdInternal_, a, svdup_f32(0.0));
> > +}
> > +
> > +struct s {
> > +    SimdBool array[1];
> > +};
> > +
> > +
> > +
> > +void foo(struct s* const work, int offset)
> > +{
> > +        svfloat32_t tz_S0;
> > +
> > +        tz_S0 = selectByMask(tz_S0, work->array[offset]);
> > +}
> > +

[-- Attachment #2: pred-addr.patch --]
[-- Type: application/octet-stream, Size: 2460 bytes --]

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index eaf2587c4817e51b47fd96d5a97965ad06deff4e..40a9d056850b91d8589a8ebf45fafff76c39964d 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -9770,7 +9770,6 @@ aarch64_classify_address (struct aarch64_address_info *info,
 			    || mode == TImode
 			    || mode == TFmode
 			    || (BYTES_BIG_ENDIAN && advsimd_struct_p));
-
   /* If we are dealing with ADDR_QUERY_LDP_STP_N that means the incoming mode
      corresponds to the actual size of the memory being loaded/stored and the
      mode of the corresponding addressing mode is half of that.  */
@@ -9779,12 +9778,14 @@ aarch64_classify_address (struct aarch64_address_info *info,
     mode = DFmode;
 
   bool allow_reg_index_p = (!load_store_pair_p
-			    && (known_lt (GET_MODE_SIZE (mode), 16)
+			    && ((vec_flags == 0
+				 && known_lt (GET_MODE_SIZE (mode), 16))
 				|| vec_flags == VEC_ADVSIMD
 				|| vec_flags & VEC_SVE_DATA));
 
-  /* For SVE, only accept [Rn], [Rn, Rm, LSL #shift] and
-     [Rn, #offset, MUL VL].  */
+  /* For SVE, only accept [Rn], [Rn, #offset, MUL VL] and [Rn, Rm, LSL #shift].
+     The latter is not valid for SVE predicates, and that's rejected through
+     allow_reg_index_p above.  */
   if ((vec_flags & (VEC_SVE_DATA | VEC_SVE_PRED)) != 0
       && (code != REG && code != PLUS))
     return false;
diff --git a/gcc/testsuite/g++.target/aarch64/sve/pr102252.C b/gcc/testsuite/g++.target/aarch64/sve/pr102252.C
new file mode 100644
index 0000000000000000000000000000000000000000..f90f1218555f0dfdb0253fe83c656ba03b1aac43
--- /dev/null
+++ b/gcc/testsuite/g++.target/aarch64/sve/pr102252.C
@@ -0,0 +1,37 @@
+/* PR target/102252.  */
+/* { dg-do assemble { target aarch64_asm_sve_ok } } */
+/* { dg-options "-march=armv8.2-a+sve -msve-vector-bits=512" } */
+
+/* We used to generate invalid assembly for SVE predicate loads.  */
+
+#include <arm_sve.h>
+
+class SimdBool
+{
+private:
+    typedef svbool_t simdInternalType_ __attribute__((arm_sve_vector_bits(512)));
+
+public:
+    SimdBool() {}
+
+    simdInternalType_ simdInternal_;
+
+};
+
+static svfloat32_t selectByMask(svfloat32_t a, SimdBool m) {
+    return svsel_f32(m.simdInternal_, a, svdup_f32(0.0));
+}
+
+struct s {
+    SimdBool array[1];
+};
+
+
+
+void foo(struct s* const work, int offset)
+{
+        svfloat32_t tz_S0;
+
+        tz_S0 = selectByMask(tz_S0, work->array[offset]);
+}
+