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

[John Baldwin <jhb@FreeBSD.org>]

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