Hi Richard,
> -----Original Message-----
> From: Richard Sandiford <richard.sandif...@arm.com>
> Sent: 13 September 2021 12:09
> To: Kyrylo Tkachov <kyrylo.tkac...@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.tkac...@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]);
> > +}
> > +
pred-addr.patch
Description: pred-addr.patch
