From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.freebsd.org (mx2.freebsd.org [IPv6:2610:1c1:1:606c::19:2]) by sourceware.org (Postfix) with ESMTPS id 131DD385737A for ; Mon, 8 Aug 2022 17:16:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 131DD385737A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=FreeBSD.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [96.47.72.80]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits)) (Client CN "mx1.freebsd.org", Issuer "R3" (verified OK)) by mx2.freebsd.org (Postfix) with ESMTPS id 4M1jYH53YBz3VBl; Mon, 8 Aug 2022 17:16:51 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [96.47.72.83]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4M1jYH4Ctrz3Qrk; Mon, 8 Aug 2022 17:16:51 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1659979011; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4/HdDealJHzA1xFaw+emWWGeCXQcSOmabqlSWhuImgQ=; b=igXLFM9PUJPMuSso7qmaKHIPfzlTYat/1wlhxiHrfMGEOl4DNeVoiyW5y93Em1ysvcBSwz EJJSWDVTQi8SAVGdaJ8vRb89h60UxdB4h78ZuVALGEMwTk4UDhxUxOM00tP05QQRGRUZig 5DpXw91oqpDisijfIke0gaRP/zlWYr4ImPEnBbVUkusubiqzpaz00qzN1Kz4R/hmRWipgZ OrpwwK6T8k5Ow/XDxkqRPlySJzGcKxPT+HsOWL8qISaIc2VV8rUpKuuI4p6/wMOUldi3O9 pt/g4G23YLLvOYhUFGbz2J1wg7IDA0U74nDLUPI/KnfOk2Lqcyxx+y9/6rtN5Q== Received: from [IPV6:2601:648:8680:ed60:d528:24a1:a7c1:6771] (unknown [IPv6:2601:648:8680:ed60:d528:24a1:a7c1:6771]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4M1jYH0cHvzymP; Mon, 8 Aug 2022 17:16:50 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: <94aab547-d6e1-94e4-49c0-d8e403a97c73@FreeBSD.org> Date: Mon, 8 Aug 2022 10:16:49 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [PATCH 2/4] gdb, gdbserver: Add AMX registers. Content-Language: en-US To: "Willgerodt, Felix" , Pedro Alves , "gdb-patches@sourceware.org" References: <20220506121226.137608-1-felix.willgerodt@intel.com> <20220506121226.137608-3-felix.willgerodt@intel.com> <8059d2c0-9b9d-e420-fe95-bc4150dfa164@palves.net> From: John Baldwin In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1659979011; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=4/HdDealJHzA1xFaw+emWWGeCXQcSOmabqlSWhuImgQ=; b=kWsYiy6TS6oqbZQNFaq1gIB/87vySa195XtcZmhukwaOXE0pWOkswkea+z4JHM8affYqhL n4Xq4N/pKnQxVNJg3hm2Oe3vHupmagYsnJ50J1wfz4OLIUTMPp6HcorTcgc27KMh4LMNxR ul7xOfD4RPj1V5RKWIWNAEBZqXCq7NZ/4LSzysKTFJGC0rsiKSypLiU1SMmlMl41W97dbv 8F0TMj1P57hez63FDJ97qFG3lTCi485B6PNmjuKuVxhqGHAAtYZ6+2cjsQl81FHwlWx5nZ xpb/aQkrT2ioJDgwVJSEQ9qqRHFUx4/+u6853WREyTcafRBzYJIneA1Di8h44Q== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1659979011; a=rsa-sha256; cv=none; b=liILZaNLVFNtnQDS2LpddWN6MjRxa96CwG33zAOih0YnfdRCVOtqL1DotNXVQ3NI7DJgbg 0nd0qaJvnRAZjhwa/I6Tgd8K4UyOS5QT0L43gfuWr9Y2qHc9wgc8dgDYzkWs4FegG1mcZ5 VZDook/QDeFQaF9/plCcFg+djotzgie76jCIbh33gN+jKrbtKOqoz7RC9NouC7G8UbVoqz xEDKoFfENvF5c/DVQZg3fvNximvh5t7lPqdYA4yiIEO7dRhRslrTZ1pG14JHH/o0+x/gBT 3I1SADGXcrzaWU/jJFxrwUfXfH1dw3MD62TbomvA1GeP1fvGzKIWCddRytxrtw== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, NICE_REPLY_A, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Aug 2022 17:16:56 -0000 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 ; gdb-patches@sourceware.org >> Subject: RE: [PATCH 2/4] gdb, gdbserver: Add AMX registers. >> >>> -----Original Message----- >>> From: Pedro Alves >>> Sent: Montag, 27. Juni 2022 20:12 >>> To: Willgerodt, Felix ; 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