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

Alan Hayward
2018-06-07T09:34:56Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3093 bytes --]

Thanks for the review. Updated as below and pushed.

> On 6 Jun 2018, at 23:17, Simon Marchi <simon.marchi@ericsson.com> wrote:
>
> 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.

Updated to use AARCH64_V_REGS_NUM.

I originally used 32 because I was matching the style of the code above which
uses 32. Agreed in this case makes more sense to use AARCH64_V_REGS_NUM.

Didn’t add the func, as here we’re checking against AARCH64_SVE_V0_REGNUM
and not AARCH64_V0_REGNUM.

>
>> /* 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.

Not changed.

Make sense, but regcache is already being passed into aarch64_pseudo_write (which
the function type is defined in gdbarch). Easier to just pass it straight down
given the compiler would have already have put gdbarch in a register.

>
>> {
>> - 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.
>

Updated to gdb_static_assert. Left both of them in the code at the same place (there’s
one in the read func and one in the write func).

I wanted something explicit in this function because these are (I think) the only
two functions that rely on the defines being the same.

Thanks,
Alan.

\x16º&Öéj×!zÊÞ¶êç×4Û‰b²Ö«r\x18\x1dn–­r\x17¬