From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 204013858D39 for ; Mon, 27 Feb 2023 14:56:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 204013858D39 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 034F3C14; Mon, 27 Feb 2023 06:57:38 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.99.50]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EA6053F67D; Mon, 27 Feb 2023 06:56:53 -0800 (PST) From: Richard Sandiford To: YunQiang Su Mail-Followup-To: YunQiang Su ,binutils@sourceware.org, syq@debian.org, macro@orcam.me.uk, xry111@xry111.site, jiaxun.yang@flygoat.com, richard.sandiford@arm.com Cc: binutils@sourceware.org, syq@debian.org, macro@orcam.me.uk, xry111@xry111.site, jiaxun.yang@flygoat.com Subject: Re: [PATCH] MIPS: make mipsisa32 and mipsisa64 link more systematic References: <20230207024424.4000862-1-yunqiang.su@cipunited.com> <20230221040650.2337395-1-yunqiang.su@cipunited.com> Date: Mon, 27 Feb 2023 14:56:52 +0000 In-Reply-To: <20230221040650.2337395-1-yunqiang.su@cipunited.com> (YunQiang Su's message of "Tue, 21 Feb 2023 12:06:50 +0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-34.8 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Sorry for the slow review, been away. YunQiang Su writes: > Introduce `static const struct mips_mach_extension mips_mach_32_64[]` > and `mips_mach_extends_32_64 (unsigned long base, unsigned long extension)`, > to make mipsisa32 and mipsisa64 interlink more systemtic. > > Normally, the ISA mipsisa64rN has two subset: mipsisa64r(N-1) and > mipsisa32rN. `mips_mach_extensions` can hold only mipsisa64r(N-1), > so we need to introduce a new instruction `mips_mach_32_64`, which holds the pair 32vs64. > > Note: R6 is not compatible with pre-R6. > > bfd/ChangeLog: > > * elfxx-mips.c (mips_mach_extends_p): make mipsisa32 and > mipsisa64 interlink more systematic. > (mips_mach_32_64): new struct added. > (mips_mach_extends_32_64): new function added. > --- > bfd/elfxx-mips.c | 38 ++++++++++++++++++++++++++++---------- > 1 file changed, 28 insertions(+), 10 deletions(-) > > diff --git a/bfd/elfxx-mips.c b/bfd/elfxx-mips.c > index e9fb61ff9e7..e9480ea3954 100644 > --- a/bfd/elfxx-mips.c > +++ b/bfd/elfxx-mips.c > @@ -14505,6 +14505,16 @@ struct mips_mach_extension > unsigned long extension, base; > }; > > +/* An array describing how BFD machines relate to one another. The entries > + are ordered topologically. */ I don't this comment really applies to the new array. How about: /* An array that maps 64-bit architectures to the corresponding 32-bit architectures. */ > +static const struct mips_mach_extension mips_mach_32_64[] = > +{ > + { bfd_mach_mipsisa64r6, bfd_mach_mipsisa32r6 }, > + { bfd_mach_mipsisa64r5, bfd_mach_mipsisa32r5 }, > + { bfd_mach_mipsisa64r3, bfd_mach_mipsisa32r3 }, > + { bfd_mach_mipsisa64r2, bfd_mach_mipsisa32r2 }, > + { bfd_mach_mipsisa64, bfd_mach_mipsisa32 } > +}; > > /* An array describing how BFD machines relate to one another. The entries > are ordered topologically with MIPS I extensions listed last. */ > @@ -14584,27 +14594,35 @@ static const struct mips_mach_extension mips_mach_extensions[] = > > /* Return true if bfd machine EXTENSION is an extension of machine BASE. */ > > +static bool > +mips_mach_extends_32_64 (unsigned long base, unsigned long extension) > +{ > + size_t i; > + > + if (extension == base) > + return true; > + > + for (i = 0; i < ARRAY_SIZE (mips_mach_64_32); i++) > + if (extension == mips_mach_32_64[i].extension) > + return base == mips_mach_32_64[i].base; > + > + return false; > +} > + I think this should go before the "Return true if..." comment and should have its own comment. How about: /* Return true if bfd machine EXTENSION is the same as BASE, or if EXTENSION is the 64-bit equivalent of a 32-bit BASE. */ OK with those changes, thanks. Richard > static bool > mips_mach_extends_p (unsigned long base, unsigned long extension) > { > size_t i; > + unsigned long extension1 = extension; > > - if (extension == base) > - return true; > - > - if (base == bfd_mach_mipsisa32 > - && mips_mach_extends_p (bfd_mach_mipsisa64, extension)) > - return true; > - > - if (base == bfd_mach_mipsisa32r2 > - && mips_mach_extends_p (bfd_mach_mipsisa64r2, extension)) > + if (mips_mach_extends_32_64 (base, extension)) > return true; > > for (i = 0; i < ARRAY_SIZE (mips_mach_extensions); i++) > if (extension == mips_mach_extensions[i].extension) > { > extension = mips_mach_extensions[i].base; > - if (extension == base) > + if (mips_mach_extends_32_64 (base, extension)) > return true; > }