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

[John Baldwin <jhb@FreeBSD.org>]

On 5/9/22 12:04 AM, Willgerodt, Felix wrote:
>> -----Original Message-----
>> From: John Baldwin <jhb@FreeBSD.org>
>> Sent: Freitag, 6. Mai 2022 18:18
>> To: Willgerodt, Felix <felix.willgerodt@intel.com>; gdb-
>> patches@sourceware.org
>> Subject: Re: [PATCH 2/4] gdb, gdbserver: Add AMX registers.
>>
>> On 5/6/22 5:12 AM, Felix Willgerodt via Gdb-patches wrote:
>>> Advanced Matrix Extensions (AMX) adds one 64 byte TILECFG register and
>>> eight 1024 byte tile registers TMM0, TMM1, ..., TMM7.  The tile registers
>>> each represent a matrix, whose dimensions are configured via TILECFG.
>>> In XSAVE, all tiles are represented in the 8kB TILEDATA section.
>>>
>>> Future AMX platforms are free to add new palettes, which are
>>> run-time configurable partitionings of the TILEDATA space.
>>> Currently only palette 0 (initialized zero state) and palette 1 exist.
>>> New palettes might change any of the following parameters, which are
>> defined
>>> in the palette_table (which can be accessed via CPUID):
>>>
>>> define palette_table[id]:
>>> 	uint16_t total_tile_bytes
>>> 	uint16_t bytes_per_tile
>>> 	uint16_t bytes_per_row
>>> 	uint16_t max_names
>>> 	uint16_t max_rows
>>>
>>> More information about AMX can be found in the Intel(R) Architecture
>>> Instruction Set Extensions Programming Reference, May 2021.
>>>
>>> The $tilecfg register is implemented as a pseudo register.  For convenience
>>> it is partitioned as a struct, representing the single configuration options
>>> as members.  It doesn't show reserved space, as structs can only contain
>>> existing data types.  To also be able to show the full register, $tilecfg_raw
>>> is implemented as a uint512 value.
>>>
>>> The $tmm0-7 registers are also represented as pseudo registers.  This
>> allows
>>> to only show the actually configured matrix and to omit filling zeros, which
>>> greatly increases readability on smaller matrices.  A raw $tiledata register
>>> is implemented as the base for the pseudo registers.
>>>
>>> When developing this we also considered updating the target description
>> at
>>> runtime to achieve the dynamic sizing.  This however would have required
>>> extensive changes to the writing and reading from/to XSAVE.  And it
>> wouldn't
>>> work with gdbserver easily, as there currently is no infrastructure to keep
>>> XML target descriptions in sync after the initial transfer.
>>> ---
>>> diff --git a/gdb/i386-linux-tdep.h b/gdb/i386-linux-tdep.h
>>> index 6b3555aa3ea..705c7bcd602 100644
>>> --- a/gdb/i386-linux-tdep.h
>>> +++ b/gdb/i386-linux-tdep.h
>>> @@ -29,7 +29,7 @@
>>>    /* Register number for the "orig_eax" pseudo-register.  If this
>>>       pseudo-register contains a value >= 0 it is interpreted as the
>>>       system call number that the kernel is supposed to restart.  */
>>> -#define I386_LINUX_ORIG_EAX_REGNUM (I386_PKRU_REGNUM + 1)
>>> +#define I386_LINUX_ORIG_EAX_REGNUM
>> (I386_AMX_TILEDATA_REGNUM + 1)
>>>
>>>    /* Total number of registers for GNU/Linux.  */
>>>    #define I386_LINUX_NUM_REGS (I386_LINUX_ORIG_EAX_REGNUM + 1)
>>> diff --git a/gdb/i386-tdep.c b/gdb/i386-tdep.c
>>> index 8501e12e241..921b24ab60f 100644
>>> --- a/gdb/i386-tdep.c
>>> +++ b/gdb/i386-tdep.c
>>> @@ -3307,6 +3389,142 @@ i386_mmx_type (struct gdbarch *gdbarch)
>>>      return tdep->i386_mmx_type;
>>>    }
>>>
>>> +/* Construct vector type for TMM registers.  */
>>> +
>>> +static struct type *
>>> +i386_tmm_type (struct gdbarch *gdbarch)
>>> +{
>>> +  i386_gdbarch_tdep *tdep = (i386_gdbarch_tdep *) gdbarch_tdep
>> (gdbarch);
>>> +
>>> +  if (!tdep->i386_tmm_type)
>>> +    {
>>> +      const struct builtin_type *bt = builtin_type (gdbarch);
>>> +
>>> +      uint8_t bytes_per_row = tilecfg_reg::MAX_BYTES_PER_ROW;
>>> +      uint8_t max_rows = tilecfg_reg::MAX_ROWS;
>>> +
>>> +      /* The type we're building is this:  */
>>> +#if 0
>>> +      union __gdb_builtin_type_matrix1024i
>>> +      {
>>> +	int8_t m_int8[max_rows][bytes_per_row];
>>> +	uint8_t m_uint8[max_rows][bytes_per_row];
>>> +	int32_t m_int32[max_rows][bytes_per_row/4];
>>> +	bfloat16_t m_bfloat16[max_rows][bytes_per_row/2];
>>> +	float m_int32[max_rows][bytes_per_row/4];
>>> +      };
>>> +#endif
>>
>> I think it might be better to put this inside of the comment instead of using
>> #if 0
>> as a reader might think that code under #if 0 might be intended to be used in
>> the
>> actual source under some circumstance (e.g. it was old code disabled but not
>> removed, or it is some kind of WIP that will be enabled in the future), but this
>> is clearly documentation that will never be compiled as part of GDB itself.
>>
>> (And I think this #if 0 pattern is in some other places in the patch as well?)
>>
>> --
>> John Baldwin
> 
> Thanks for the feedback. I understand your point. But all pseudo register type
> functions in this file (e.g. i386_zmm_type, i386_zmm_type and i386_bnd_type)
> use this style of "comment". I don't know the background and am a bit
> reluctant to change the style in my patch series. I think that should be done
> separately.

Oh, yes, sorry, if it is consistent with existing style then best to leave it
as-is.

-- 
John Baldwin