Re: [PATCH 1/8] Add Aarch64 SVE target description

Simon Marchi
2018-05-31T11:10:16Z

On 2018-05-11 06:52 AM, Alan Hayward wrote:
> This patch adds the SVE target description. However, no code will
> yet use it - that comes in the later patches.
>
> The create_feature_aarch64_sve function is not generated from XML.
> This is because we need to know the sve vector size (VQ) in order
> to size the registers correctly.
>
> A VQ of 0 is used when the hardware does not support SVE.
> (SVE hardware will always have a valid vector size). I considered
> using a bool to indicate SVE in addition to the VQ. Whilst this
> may be slightly more readable initially, I think it's a little
> odd to have two variables, eg:
> aarch64_create_target_description (bool sve_supported, long vq)
>
> Alan.

Hi Alan,

This patch LGTM, I just noted some nits.

> /* Initialize the current architecture based on INFO. If possible,
> @@ -2864,7 +2875,8 @@ aarch64_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>
> /* Ensure we always have a target descriptor. */
> if (!tdesc_has_registers (tdesc))
> - tdesc = aarch64_read_description ();
> + /* SVE is not yet supported. */
> + tdesc = aarch64_read_description (0);

When there there is a comment above the single statement branch, braces become required:

if (!tdesc_has_registers (tdesc))
{
/* SVE is not yet supported. */
tdesc = aarch64_read_description (0);
}

> diff --git a/gdb/arch/aarch64.c b/gdb/arch/aarch64.c
> index b85e460b6b..d1ec5cedf8 100644
> --- a/gdb/arch/aarch64.c
> +++ b/gdb/arch/aarch64.c
> @@ -21,11 +21,13 @@
>
> #include "../features/aarch64-core.c"
> #include "../features/aarch64-fpu.c"
> +#include "../features/aarch64-sve.c"
>
> -/* Create the aarch64 target description. */
> +/* Create the aarch64 target description. A non zero VQ value indicates both
> + the presence of SVE and the SVE vector quotient. */

What does "SVE vector quotient" mean? Is there maybe a simpler way to say it?

Could you move this comment to the .h and put

/* See arch/aarch64.h. */

here?

> diff --git a/gdb/features/aarch64-sve.c b/gdb/features/aarch64-sve.c
> new file mode 100644
> index 0000000000..6442640a73
> --- /dev/null
> +++ b/gdb/features/aarch64-sve.c
> @@ -0,0 +1,158 @@
> +/* Copyright (C) 2017 Free Software Foundation, Inc.

2018

Thanks,

Simon