Hi,

Each of the comments on the previous version of the patch have been
addressed.

Ok for master?

Thanks,
Jonathan


From: Richard Sandiford <richard.sandiford@arm.com>
Sent: 22 October 2021 16:13
To: Jonathan Wright <Jonathan.Wright@arm.com>
Cc: gcc-patches@gcc.gnu.org <gcc-patches@gcc.gnu.org>; Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
Subject: Re: [PATCH 4/6] aarch64: Add machine modes for Neon vector-tuple types 
 
Thanks a lot for doing this.

Jonathan Wright <Jonathan.Wright@arm.com> writes:
> @@ -763,9 +839,16 @@ aarch64_lookup_simd_builtin_type (machine_mode mode,
>      return aarch64_simd_builtin_std_type (mode, q);
>  
>    for (i = 0; i < nelts; i++)
> -    if (aarch64_simd_types[i].mode == mode
> -     && aarch64_simd_types[i].q == q)
> -      return aarch64_simd_types[i].itype;
> +    {
> +      if (aarch64_simd_types[i].mode == mode
> +       && aarch64_simd_types[i].q == q)
> +     return aarch64_simd_types[i].itype;
> +      else if (aarch64_simd_tuple_types[i][0] != NULL_TREE)

Very minor (sorry for not noticing earlier), but: the “else” is
redundant here.

> +     for (int j = 0; j < 3; j++)
> +       if (TYPE_MODE (aarch64_simd_tuple_types[i][j]) == mode
> +           && aarch64_simd_types[i].q == q)
> +         return aarch64_simd_tuple_types[i][j];
> +    }
>  
>    return NULL_TREE;
>  }
> diff --git a/gcc/config/aarch64/aarch64-simd.md b/gcc/config/aarch64/aarch64-simd.md
> index 48eddf64e05afe3788abfa05141f6544a9323ea1..0aa185b67ff13d40c87db0449aec312929ff5387 100644
> --- a/gcc/config/aarch64/aarch64-simd.md
> +++ b/gcc/config/aarch64/aarch64-simd.md
> @@ -6636,162 +6636,165 @@
>  
>  ;; Patterns for vector struct loads and stores.
>  
> -(define_insn "aarch64_simd_ld2<mode>"
> -  [(set (match_operand:OI 0 "register_operand" "=w")
> -     (unspec:OI [(match_operand:OI 1 "aarch64_simd_struct_operand" "Utv")
> -                 (unspec:VQ [(const_int 0)] UNSPEC_VSTRUCTDUMMY)]
> -                UNSPEC_LD2))]
> +(define_insn "aarch64_simd_ld2<vstruct_elt>"
> +  [(set (match_operand:VSTRUCT_2Q 0 "register_operand" "=w")
> +     (unspec:VSTRUCT_2Q [
> +       (match_operand:VSTRUCT_2Q 1 "aarch64_simd_struct_operand" "Utv")]
> +       UNSPEC_LD2))]
>    "TARGET_SIMD"
>    "ld2\\t{%S0.<Vtype> - %T0.<Vtype>}, %1"
>    [(set_attr "type" "neon_load2_2reg<q>")]
>  )
>  
> -(define_insn "aarch64_simd_ld2r<mode>"
> -  [(set (match_operand:OI 0 "register_operand" "=w")
> -       (unspec:OI [(match_operand:BLK 1 "aarch64_simd_struct_operand" "Utv")
> -                   (unspec:VALLDIF [(const_int 0)] UNSPEC_VSTRUCTDUMMY) ]
> -                  UNSPEC_LD2_DUP))]
> +(define_insn "aarch64_simd_ld2r<vstruct_elt>"
> +  [(set (match_operand:VSTRUCT_2QD 0 "register_operand" "=w")
> +     (unspec:VSTRUCT_2QD [
> +       (match_operand:VSTRUCT_2QD 1 "aarch64_simd_struct_operand" "Utv")]
> +          UNSPEC_LD2_DUP))]

Sorry again for missing this, but the ld2rs, ld3rs and ld4rs should
keep their BLKmode arguments, since they only access 2, 3 or 4
scalar memory elements.

> @@ -7515,10 +7605,10 @@
>  )
>  
>  (define_insn_and_split "aarch64_combinev16qi"
> -  [(set (match_operand:OI 0 "register_operand" "=w")
> -     (unspec:OI [(match_operand:V16QI 1 "register_operand" "w")
> -                 (match_operand:V16QI 2 "register_operand" "w")]
> -                UNSPEC_CONCAT))]
> +  [(set (match_operand:V2x16QI 0 "register_operand" "=w")
> +     (unspec:V2x16QI [(match_operand:V16QI 1 "register_operand" "w")
> +                      (match_operand:V16QI 2 "register_operand" "w")]
> +                     UNSPEC_CONCAT))]

Just realised that we can now make this a vec_concat, since the
modes are finally self-consistent.

No need to do that though, either way is fine.

Looks good otherwise.

Richard