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..e6253edeb55cdcc3dbc7303e03bad26dd519c4b1
> 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.
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..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]);
> +}
> +
