From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-sender-0.a4lg.com (mail-sender.a4lg.com [153.120.152.154]) by sourceware.org (Postfix) with ESMTPS id 8EF803858D1E for ; Fri, 20 Oct 2023 02:53:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8EF803858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=irq.a4lg.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=irq.a4lg.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8EF803858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=153.120.152.154 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697770386; cv=none; b=bqqPHiCBgr22NWVHFOnT7tTOQvdAz470wTI9PbP1TtMin0R4e0KybaO/H9kDOELuZzFDA+DGKVCpMLMp0YfxPi0oPvHE2SLa5hag1hxKrFp76q//vL7yCesCopGdpiSSEQr3IlMquX48oq8KeTCb5hOzfoMZtTWT/SZxJaydOb4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697770386; c=relaxed/simple; bh=1GNR/ubeKWhHbxhj6rjjGHiWoJ1aR/KICK0q8FTUbbk=; h=DKIM-Signature:Message-ID:Date:Mime-Version:Subject:To:From; b=GvrZYZetsmg3X83O/vMGmGGFhXhsImYupvRW9pvj7CJwals+ANrYr2B9Qm3DLa+E9119y/3/O58i1kAg5Gpf86J736fk5wk8jutIUQsjNiQHV0lhsCns7vkxzjfumNZ4HDr39bKwCAUECTaYtBgQMirwLKPg+ofxd9sz2JGNoLc= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from [127.0.0.1] (localhost [127.0.0.1]) by mail-sender-0.a4lg.com (Postfix) with ESMTPSA id 77B74300089; Fri, 20 Oct 2023 02:53:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=irq.a4lg.com; s=2017s01; t=1697770380; bh=sfWXIBSIcxX5c3j3CIhhCmkHUhqes4+r0jRubUF123A=; h=Message-ID:Date:Mime-Version:Subject:To:References:From: In-Reply-To:Content-Type:Content-Transfer-Encoding; b=YM8m8zpj4hExGQBYLz+6+0XwZvIlhX3vf2TNu1iZOfeGncErsurM9DAljb6frzdS/ quElsnGRWVoreUki+Ra0Vy3yiUTEGBzY0J+Flg27qXYr5DtrVFJypvvwWaB3O4lwu/ tQRhC0JcFMNf6atU2Jzf+De2+xKsJfV06tqt5tRU= Message-ID: Date: Fri, 20 Oct 2023 11:52:58 +0900 Mime-Version: 1.0 Subject: Re: [PING^1][RFC PATCH 0/2] RISC-V: Add 'Zicntr' and 'Zihpm' support with compatibility measures Content-Language: en-US To: Nelson Chu , Binutils References: <3a56687f-e513-4662-aca9-f3d2a6587acb@irq.a4lg.com> From: Tsukasa OI In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-6.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,SPF_HELO_NONE,SPF_PASS,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: On 2023/10/19 17:33, Nelson Chu wrote: > I hope we can remove the support of -misa-spec and -mpriv-spec from > binutils, and then simply support the newest extension versions and CSRs > by default.  These features are good until spec 20191213 and privileged > spec 2.2, but they are harmful later, since spec no longer needs its own > versions.  Continuing to build on all this mistake will make it all look > worse, so I hope we can stop supporting -misa-spec/-mpriv-spec to take > care of the compatibility issues.  Only in this way can we prevent us > from wasting time on these faults caused by me many years ago. > > Thanks > Nelson I could not be more appreciative of your input. I mean, I must thank you because I can get rid of my garbage and make another better one from scratch. It's better if we don't have to depend on -misa-spec and I think... softer approach works. I'm making the new version based on the following concept: 1. 'I' does not imply 'Zicntr' and 'Zihpm' in any ISA spec. 2. Using 'Zicntr' pseudoinstructions without the extension causes a warning (but not an error; this is new). Using 'Zihpm' CSRs without the extension causes a warning when -mcsr-check is enabled (as usual). 3. We have one extension version entry for each of them ('Zicntr' and 'Zihpm': ISA_SPEC_DRAFT, version 2.0) So, my next approach will be, compliant as possible, except its violation does not make a hard error (to avoid compatibility issues; I don't want to repeat 'Zicsr' mess again). Do this work for you? Thanks, Tsukasa > > On Thu, Oct 19, 2023 at 3:58 PM Tsukasa OI > wrote: > > Ping to all RISC-V folks, > > Though there are more possible ways than described in the original > e-mail, at least I would like to hear from you all what do you want when > we are going to support now ratified 'Zicntr' and 'Zihpm' extensions. > > > Additional Note 1. When "zicntr" and/or "zihpm" are explicitly stated? > > My initial RFC PATCH suppresses output of Zicntr and Zihpm extensions > unless the version number is specified explicitly (there's room to fix > this issue). > > Additional Note 2. Warn, instead of making errors? > > We have an option to make warnings when 'Zicntr' instructions are used > without the 'Zicntr' extension (when we are going to "compliant mode"). > That will add a special code path but can be less breaking for users. > > > Sincerely, > Tsukasa > > On 2023/08/08 12:17, Tsukasa OI wrote: > > Hi, > > > > The role of this patch set is very simple: implement recently ratified > > 'Zicntr' (basic counters and timers) and 'Zihpm' (hardware performance > > counters) extensions, formerly a part of the 'I' extension, > version 2.0. > > > > Since some draft extensions depend on those extensions, > implementing counter > > extensions (as a part of data structure) is becoming mandatory. > > > > However, we need to be *very* careful to the implementation.  Because > > CSRs and pseudoinstructions for those extensions were a part of > 'I' and > > more importantly, the first version which separated counters from > the 'I' > > extension, did not give counters extension names.  So, we needed to > > implement such CSRs and pseudoinstructions as a part of 'I' > (continuously). > > > > > > Not breaking the compatibility here is vital (the only exception > might be > > the new ratified ISA after version 20191213).  So, I implemented those > > extensions not to break anything as possible. > > > > The basic idea is, an extension (riscv_subset_t) with an unknown > version is > > not emitted to an object file (ELF attributes, mapping symbols > etc...). > > > > The default mode for existing ISAs is the compatibility mode > (Option 1). > > > > > > [Option 1: Compatibility Mode] > > > > In the compatibility mode (as default), > > > > 1.  'I' implies 'Zicntr' and 'Zihpm'. > > 2.  'Zicntr' and 'Zihpm' DO NOT imply 'Zicsr'. > > 3.  'Zicntr' and 'Zihpm' don't have version information. > > > > (2.) is the point.  The ratified document says 'Zicntr' and > 'Zihpm' depend > > on the 'Zicsr' extension but if we do it unconditionally, that > would mean > > that the 'I' extension indirectly depends on 'Zicsr' (because of > 1.), making > > a difference from the ratified 'I' extension version 2.1.  In > order to keep > > the compatibility, making 'Zicntr' and 'Zihpm' against the > documentation was > > (sadly) necessary. > > > > In the compatibility mode, code like: > > > >> riscv_subset_supports(&rps, "zicntr") > > > > will return true.  Because, even that the version information is > missing, > > the 'Zicntr' extension exists in the riscv_subset_list_t.  But an > extension > > with no version means, it will not be a part of the architectural > string > > emitted as a part of an object file. > > > > > > [Option 2: Compliant Mode] > > > > We can continue this forever but we have another option.  Break false > > dependency when a new ISA (with its version) is ratified and in > that time, > > require those extensions separately like -march=..._zicntr_zihpm. > > > > In the compliant mode: > > > > 1.  'I' DOES NOT imply 'Zicntr' and 'Zihpm'. > > 2.  'Zicntr' and 'Zihpm' DO imply 'Zicsr'. > > 3.  'Zicntr' and 'Zihpm' have its version information > >     (ratified version 2.0 or possibly a later version) > > > > Note that (1.) and (2.) are very opposite from the compatibility mode. > > > > In this mode, it is compliant to the specification completely. > > > > > > > > > > [Implementing an Option on future ISAs] > > > > Assume that we have a new ISA specification class, > ISA_SPEC_CLASS_2024XXXX. > > We can choose which option to implement as follows. > > > > > > [Option 1: Compatibility Mode] > > > > 1. Change the contents of check_implicit_compat_counter_from_i. > > > >>  /* Old.  */ > >>  return *rps->isa_spec <= ISA_SPEC_CLASS_20191213; > > > >>  /* New.  */ > >>  return *rps->isa_spec <= ISA_SPEC_CLASS_2024XXXX; > > > > Note that the reason we are looking for the ISA specification > (instead of > > the version of the 'I' extension) is, the 'I' extension version 2.1 is > > unlikely to change even when the new ISA specification is ratified. > > I assume that two behaviors (option 1 and 2) share the same 'I' > version 2.1. > > > > If the version of the 'I' extension changes (even if no *actual* > changes > > are made) in the new unprivileged ISA version, that would be a bit > simpler. > > > > 2. Add following entries to riscv_supported_std_z_ext. > > > > Make sure that they don't have the version number. > > > >>  /* ...  */ > >>  {"zicntr",          ISA_SPEC_CLASS_2024XXXX,        > RISCV_UNKNOWN_VERSION, RISCV_UNKNOWN_VERSION,  0 }, /* Compat.  */ > >>  /* ...  */ > >>  {"zihpm",           ISA_SPEC_CLASS_2024XXXX,        > RISCV_UNKNOWN_VERSION, RISCV_UNKNOWN_VERSION,  0 }, /* Compat.  */ > > > > > > [Option 2: Compliant Mode] > > > > 1. DO NOT change the contents of check_implicit_compat_counter_from_i. > > 2. Add following entries to riscv_supported_std_z_ext. > > > > Make sure that they DO have the version number. > > > >>  /* ...  */ > >>  {"zicntr",          ISA_SPEC_CLASS_2024XXXX,        2, 0,  0 }, > >>  /* ...  */ > >>  {"zihpm",           ISA_SPEC_CLASS_2024XXXX,        2, 0,  0 }, > > > > ...and for compatibility, we need to slightly modify riscv-dis.c.  The > > disassembler defaults to the latest non-draft ISA and there's > currently no > > ways to change it.  We need to make changes to riscv-dis.c by either: > > > > 1.  Entering special compatibility mode (even on the latest ISA, > enable > >     'Zicntr' extension ['Zihpm' is not necessary on the > disassembler]) or > > 2.  Enabling to change the ISA version from disassembler options. > >     Like my long proposed disassembler changes: adding overridable > "arch" > >     and "priv-spec" options, we have an option to change the ISA > using the > >     disassembler option. > >    >   > > > > > > > In either case, this patch set leaves both options to implement yet > > supporting 'Zicntr' and 'Zihpm' extensions directly. > > > > > > > > > > Along with those changes (in PATCH 2), PATCH 1 makes possible to > consider > > implication by using *more* information than before.  This is > practically > > the same as: > > >, > > a patch to demonstrate how to implement the 'Zce' superset extension > > ('Zcf' must be implied by 'Zce' ONLY IF 'F' is ALSO enabled). > > > > The only difference is a minor type change as follows (to enable using > > the "riscv_subset_supports" function). > > > > *   Before: "const riscv_parse_subset_t *" > > *   After:  "      riscv_parse_subset_t *" > > > > > > > > > > Is there any other options?  Should I simplify the patch assuming > we choose > > the compatibility mode (Option 1) forever? > > > > In any case, I would like to hear your thoughts. > > > > Thanks, > > Tsukasa > > > > > > > > > > Tsukasa OI (2): > >   RISC-V: Base for complex extension implications > >   RISC-V: Add 'Zicntr' and 'Zihpm' support with compatibility measures > > > >  bfd/elfxx-riscv.c                       |  86 +++++-- > >  gas/config/tc-riscv.c                   |  16 ++ > >  gas/testsuite/gas/riscv/march-imply-i.s |   8 + > >  include/opcode/riscv-opc.h              | 310 > ++++++++++++------------ > >  include/opcode/riscv.h                  |   1 + > >  opcodes/riscv-opc.c                     |  12 +- > >  6 files changed, 256 insertions(+), 177 deletions(-) > > > > > > base-commit: d734d43a048b33ee12df2c06c2e782887e9715f6 >