Re: [PATCH 2/4] gdb, gdbserver: Add AMX registers.

[John Baldwin <jhb@FreeBSD.org>]

On 8/8/22 2:15 AM, Willgerodt, Felix via Gdb-patches wrote:
>> -----Original Message-----
>> From: Willgerodt, Felix
>> Sent: Donnerstag, 14. Juli 2022 12:55
>> To: Pedro Alves <pedro@palves.net>; gdb-patches@sourceware.org
>> Subject: RE: [PATCH 2/4] gdb, gdbserver: Add AMX registers.
>>
>>> -----Original Message-----
>>> From: Pedro Alves <pedro@palves.net>
>>> Sent: Montag, 27. Juni 2022 20:12
>>> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
>>> patches@sourceware.org
>>> Subject: Re: [PATCH 2/4] gdb, gdbserver: Add AMX registers.
>>>
>>> Hi Felix,
>>>
>>> This largely looks good to me, though I have a couple questions.  See
>> below.
>>>
>>
>> Hi Pedro,
>>
>> Thanks for your review. Sorry for taking so long to reply, see my comments
>> below.
>>
>>> On 2022-05-06 13:12, Felix Willgerodt via Gdb-patches wrote:
>>>
>>>>
>>>> +/* A helper function to re-size AMX pseudo registers during reads.
>> Copies
>>>> +   the contents from RAW_BUF to BUF and re-sizes the value.  */
>>>
>>> I think this should say what does it mean when TILECFG is NULL.
>>
>> The next version will add a sentence.
>>
>>>> +
>>>> +static void
>>>> +amd64_tmm_resize_read (const tilecfg_reg *tilecfg, const gdb_byte
>>> *raw_buf,
>>>> +		       gdb_byte *buf, value *result_value, const int tmmnum)
>>>> +{
>>>> +  uint16_t columns = 64;
>>>> +  uint8_t rows = 16;
>>>> +
>>>> +  if (tilecfg != nullptr)
>>>> +    {
>>>> +      columns = tilecfg->bytes_per_row (tmmnum);
>>>> +      rows = tilecfg->rows (tmmnum);
>>>> +      if (columns == 0)
>>>> +	columns = 64;
>>>> +      if (rows == 0)
>>>> +	rows = 16;
>>>> +    }
>>>> +
>>>> +  gdb_assert (TYPE_LENGTH (value_type (result_value)) >= rows *
>>> columns);
>>>> +
>>>> +  /* Copy each row from raw_buf into buf.  The rows are not
>> consecutive
>>>> +     but they are on MAX_BYTES_PER_ROW * iRow position.  */
>>>> +  const gdb_byte *raw_buf_offset
>>>> +    = raw_buf + tmmnum * tilecfg->MAX_BYTES_PER_TILE;
>>>> +  for (uint8_t iRow = 0; iRow < rows; ++iRow)
>>>> +    {
>>>> +      memcpy (buf + columns * iRow,
>>>> +	      raw_buf_offset + tilecfg->MAX_BYTES_PER_ROW * iRow,
>>>> +	      columns);
>>>> +    }
>>>> +
>>>> +  /* Adjust the result_value.  The value is a union of matrices of different
>>>> +     types.  See i386_tmm_type ().  This iterates over each member and
>>>> +     adjusts the dimensions according to the type.  */
>>>> +  for (int i = 0; i < value_type (result_value)->num_fields (); ++i)
>>>> +    {
>>>> +      type *rows_type = value_type (result_value)->fields ()[i].m_type;
>>>> +      type *cols_type = rows_type->main_type->target_type;
>>>> +
>>>> +      /* Adjust array bit lengths.  */
>>>> +      rows_type->length = columns * rows;
>>>> +      cols_type->length = columns;
>>>> +
>>>> +      /* Adjust array dimensions.  */
>>>> +      rows_type->bounds ()->high.set_const_val (rows - 1);
>>>> +      int num_bytes = cols_type->main_type->target_type->length;
>>>> +      cols_type->bounds ()->high.set_const_val (columns / num_bytes -
>> 1);
>>>
>>> Does any other target do in-place type rewriting like this?
>>
>> I am not aware of anyone else that has done this exactly. ARM SVE has the
>> easier case of having only a vector, that you can just cut off or extend at the
>> end.
>>
>>
>>>   That seems fishy.
>>> What happens e.g.,
>>> to values already in the value history that were recorded before the
>>> dimensions changed, for
>>> instance?  Will they suddenly start re-printing differently / incorrectly with
>>> their type changed
>>> behind their back?
>>>
>>> Like:
>>>
>>>   (gdb) print $reg  # some register or value mapped to a register that that
>> ends
>>> up in the function above
>>>   $1 = ...  # before type changes
>>>   # something happens and the AMX type changes.
>>>   (gdb) print $reg
>>>   $2 = ...  # reflects type change
>>>   (gdb) print $1
>>>   $3 = ...  # what type does GDB use here?
>>>
>>> Do the new tests cover something like this already?
>>
>> No they don't cover this. A tilecfg change flushes the tmm register though.
>> When I set the tilecfg manually in GDB, indeed $1 changes as well.
>>
>> 	(gdb) p $tmm0.m_int8
>> 	$1 = {{5, 5, 5, 5, 6, 6, 6, 6}}
>> 	(gdb) p $tilecfg.tile0_colsb
>> 	$2 = 8
>> 	(gdb) p $tilecfg.tile0_colsb = 4
>> 	$3 = 4
>> 	(gdb) p $tmm0.m_int8
>> 	$5 = {{5, 5, 5, 5}}
>> 	(gdb) p $1
>> 	$6 = {{5, 5, 5, 5}}
>>
>> Good catch, I didn't think of this. We should fix that.
>>
>>> This may likewise affect, e.g., watchpoints and displays.
>>>
>>> I haven't traced the new code to check where do those types originally
>> come
>>> from, but maybe it
>>> would work to reuse/extend the vla support to make those types have
>>> dynamic length and
>>> bounds (TYPE_DYNAMIC_LENGTH, DYN_PROP_BYTE_SIZE, etc.).
>>
>> I have looked a bit at the dynamic length for types now, but that doesn't
>> seem to account for dimensions, just (byte) length or rank.
>> Or at least I don't see how we could use it here.
>>
>>> Or maybe just tweak these functions such that you create a new type
>>> instead of changing the
>>> original type.  I don't know how frequently the array dimentions change
>> and
>>> how open
>>> ended the dimensions are, but caching the type keyed on row/col sizes
>> may
>>> work well to
>>> spare creating too many types, or actually creating them all the time.
>>
>> I tried implementing this approach a while ago (without any type caching).
>> Having a i386_tmm_type() accept dimensions, creating the type directly.
>> And returning that instead of the manual resize.
>> The problem was that in value.c:value_fetch_lazy_register(), gdb just
>> copies the contents of NEW_VAL to VAL, assuming the same
>> type/length/dimensions. The "old" VAL comes from
>> findvar.c:value_of_register_lazy(), where it is fetched using
>> regcache.c:register_type().
>> Which looks at regcache_descr->register_type.
>> In regcache.c, I see this old comment:
>>
>>    /* Lay out the register cache.
>>
>>       NOTE: cagney/2002-05-22: Only register_type () is used when
>>       constructing the register cache.  It is assumed that the
>>       register's raw size, virtual size and type length are all the
>>       same.  */
>>
>> (What even is a virtual size?)
>>
>> I struggle to figure out how to best address this.
>> Maybe allowing for multiple entries per register in the register_type table in
>> regcache?
>> Not sure how much effort that is or if there are any other implications.
>>
>> Or I could call gdbarch_register_type in regcache.c:register_type() again?
>> Maybe only conditionally, if the register_type was marked with a dynamic
>> property?
>> Indicating that it can change at runtime and only the arch can figure it out.
>> But would that even solve the "$1 issue"?
>>
>> I am really happy about any pointers.
> 
> 
> Hi Pedro,
> 
> Did you get a chance to look at this again? I did find a fix for the
> issue you pointed out. But I am not sure if my approach is right.
> 
> Basically my fix avoids using the type caching for some pseudo regs:
> 
> --- a/gdb/regcache.c
> +++ b/gdb/regcache.c
> @@ -160,7 +160,14 @@ register_type (struct gdbarch *gdbarch, int regnum)
>     struct regcache_descr *descr = regcache_descr (gdbarch);
>   
>     gdb_assert (regnum >= 0 && regnum < descr->nr_cooked_registers);
> -  return descr->register_type[regnum];
> +
> +  /* Some architectures have variable length vector pseudo registers,
> +     whose type needs to be re-evaluated at runtime.  */
> +  struct type *t = descr->register_type[regnum];
> +  if (gdbarch_num_regs (gdbarch) < regnum && t->is_vector ())
> +    t = gdbarch_register_type (gdbarch, regnum);
> +
> +  return t;
>   }
> 
> I tried to have it like this first:
> 
> +  if (gdbarch_num_regs (gdbarch) < regnum && TYPE_DYNAMIC_LENGTH(t))
> 
> However a dynamic property needs to be objfile owned (see
> gdbtypes.c:add_dyn_prop). Which seems wrong for register types.
> Then again, I am not sure if is_vector() would be considered an acceptable
> condition.
> 
> Would this approach (disabling type caching for certain cases) be good enough?
> With this approach I can avoid the "on-the-fly" type resizing in my current patches
> and fix the $1 problem.
> 
> Thanks,
> Felix

I do think that if TYPE_DYNAMIC_LENGTH can't be used that it is probably
worth adding an explicit type flag for this case rather than overloading
is_vector.

-- 
John Baldwin