Re: [PATCH v2 01/10] Aarch64 SVE pseudo register support

Simon Marchi
2018-06-06T22:17:56Z

Hi Alan,

I have some more nits/suggestions, feel free to cherry-pick the ones you
want and push the result.

On 2018-06-06 11:16 AM, Alan Hayward wrote:
> @@ -2243,6 +2297,9 @@ aarch64_pseudo_register_reggroup_p (struct gdbarch *gdbarch, int regnum,
> return group == all_reggroup || group == vector_reggroup;
> else if (regnum >= AARCH64_B0_REGNUM && regnum < AARCH64_B0_REGNUM + 32)
> return group == all_reggroup || group == vector_reggroup;
> + else if (tdep->has_sve () && regnum >= AARCH64_SVE_V0_REGNUM
> + && regnum < AARCH64_SVE_V0_REGNUM + 32)

Here you use the magical "32" number but in aarch64_pseudo_register_name you used
AARCH64_V_REGS_NUM to refer (I think) to the same number. Would it be good to use
AARCH64_V_REGS_NUM everywhere? It might be good to extract that condition in a
function:

static bool
is_sve_regnum (int regnum)
{
return (regnum >= AARCH64_SVE_V0_REGNUM
&& regnum < AARCH64_SVE_V0_REGNUM + AARCH64_V_REGS_NUM);
}

and use it throughout.

> /* Helper for aarch64_pseudo_write. */
>
> static void
> -aarch64_pseudo_write_1 (struct regcache *regcache, int regnum_offset,
> - int regsize, const gdb_byte *buf)
> +aarch64_pseudo_write_1 (struct gdbarch *gdbarch, struct regcache *regcache,
> + int regnum_offset, int regsize, const gdb_byte *buf)

You could use the gdbarch from regcache.

> {
> - gdb_byte reg_buf[V_REGISTER_SIZE];
> unsigned v_regnum = AARCH64_V0_REGNUM + regnum_offset;
>
> + /* Enough space for a full vector register. */
> + gdb_byte reg_buf[register_size (gdbarch, AARCH64_V0_REGNUM)];
> + gdb_assert (AARCH64_V0_REGNUM == AARCH64_SVE_Z0_REGNUM);

This is checking a static assertion. You could use static_assert instead, which
produces a compilation error if false. You can either leave it here or put it
next to the aarch64_regnum definition. I have seen the same gdb_assert somewhere
else too, with a static_assert you only need one.

Thanks,

Simon