From: "Willgerodt, Felix" <felix.willgerodt@intel.com>
To: Pedro Alves <pedro@palves.net>,
"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: RE: [PATCH 2/4] gdb, gdbserver: Add AMX registers.
Date: Mon, 8 Aug 2022 09:15:59 +0000 [thread overview]
Message-ID: <MN2PR11MB4566FC59B75D938491B089298E639@MN2PR11MB4566.namprd11.prod.outlook.com> (raw)
In-Reply-To: <8059d2c0-9b9d-e420-fe95-bc4150dfa164@palves.net>
> -----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
next prev parent reply other threads:[~2022-08-08 9:16 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-06 12:12 [PATCH 0/4] Add AMX support Felix Willgerodt
2022-05-06 12:12 ` [PATCH 1/4] gdb: define int512 and uint512 as built-in types Felix Willgerodt
2022-05-06 12:19 ` Eli Zaretskii
2022-06-27 18:17 ` Pedro Alves
2022-05-06 12:12 ` [PATCH 2/4] gdb, gdbserver: Add AMX registers Felix Willgerodt
2022-05-06 12:25 ` Eli Zaretskii
2022-05-11 8:14 ` Willgerodt, Felix
2022-05-11 11:41 ` Eli Zaretskii
2022-06-27 18:16 ` Pedro Alves
2022-06-27 18:24 ` Eli Zaretskii
2022-06-27 19:15 ` Pedro Alves
2022-06-28 12:09 ` Eli Zaretskii
2022-06-28 13:35 ` Pedro Alves
2022-05-06 16:17 ` John Baldwin
2022-05-09 7:04 ` Willgerodt, Felix
2022-05-09 16:31 ` John Baldwin
2022-06-27 18:12 ` Pedro Alves
2022-07-14 10:54 ` Willgerodt, Felix
2022-07-15 11:51 ` Willgerodt, Felix
2022-08-08 9:15 ` Willgerodt, Felix [this message]
2022-08-08 17:16 ` John Baldwin
2022-05-06 12:12 ` [PATCH 3/4] gdb, gdbserver: Allocate only a sane amount of buffer when fetching registers Felix Willgerodt
2022-05-06 16:08 ` John Baldwin
2022-05-09 7:04 ` Willgerodt, Felix
2022-06-27 18:30 ` Pedro Alves
2022-05-06 12:12 ` [PATCH 4/4] gdb: Clear tilecfg.start_row for any PC modification Felix Willgerodt
2022-06-27 18:55 ` Pedro Alves
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MN2PR11MB4566FC59B75D938491B089298E639@MN2PR11MB4566.namprd11.prod.outlook.com \
--to=felix.willgerodt@intel.com \
--cc=gdb-patches@sourceware.org \
--cc=pedro@palves.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).