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

["Willgerodt, Felix via Gdb-patches" <gdb-patches@sourceware.org>]

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


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928