On Tue, Mar 16, 2021 at 9:43 AM Richard Sandiford via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Kyrylo Tkachov <kyrylo.tkac...@arm.com> writes:
> > Hi Jakub,
> >
> >> -----Original Message-----
> >> From: Jakub Jelinek <ja...@redhat.com>
> >> Sent: 13 March 2021 20:34
> >> To: Richard Sandiford <richard.sandif...@arm.com>; Richard Earnshaw
> >> <richard.earns...@arm.com>; Kyrylo Tkachov <kyrylo.tkac...@arm.com>
> >> Cc: gcc-patches@gcc.gnu.org
> >> Subject: [PATCH] aarch64: Fix up
> >> aarch64_simd_clone_compute_vecsize_and_simdlen [PR99542]
> >>
> >> Hi!
> >>
> >> As the patch shows, there are several bugs in
> >> aarch64_simd_clone_compute_vecsize_and_simdlen.
> >> One is that unlike for function declarations that aren't definitions
> >> it completely ignores argument types. Such decls don't have
> >> DECL_ARGUMENTS,
> >> but we can walk TYPE_ARG_TYPES instead, like the i386 backend does or like
> >> the simd cloning code in the middle end does too.
> >>
> >> Another problem is that it checks types of uniform arguments. That is
> >> unnecessary, uniform arguments are passed the way it normally is, it is
> >> a scalar argument rather than vector, so there is no reason not to support
> >> uniform argument of different size, or long double, structure etc.
> >>
> >> Fixed thusly, bootstrapped/regtested on aarch64-linux, ok for trunk?
> >
> > I don't have much experience in this part of GCC and I think you're best
> > placed to make a judgement on the implementation of this hook.
> > It's okay from my perspective (it doesn't look like it would break any SVE
> > logic), but if you'd like to wait for a review from Richard I won't object.
>
> Yeah, it looks good to me too, sorry for the slow response.
>
> IMO:
>
> tree type_arg_types = TYPE_ARG_TYPES (TREE_TYPE (node->decl));
> bool decl_arg_p = (node->definition || type_arg_types == NULL_TREE);
>
> for (t = (decl_arg_p ? DECL_ARGUMENTS (node->decl) : type_arg_types), i = 0;
> t && t != void_list_node; t = TREE_CHAIN (t), i++)
> {
> tree arg_type = decl_arg_p ? TREE_TYPE (t) : TREE_VALUE (t);
>
> shows we have an interface problem here -- we shouldn't need something this
> convoluted to walk through an argument list. But that's not your problem
> or a problem with the patch.
The caller side should IMHO always look at TYPE_ARG_TYPES since
that's what determines the ABI. But IIRC there were problems in the past
that TYPE_ARG_TYPES might be NULL but DECL_ARGUMENTS not?!
>
> Thanks,
> Richard
