Re: [PING^1][RFC PATCH 0/2] RISC-V: Add 'Zicntr' and 'Zihpm' support with compatibility measures

[Nelson Chu <nelson@rivosinc.com>]

[-- Attachment #1: Type: text/plain, Size: 8802 bytes --]

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

On Thu, Oct 19, 2023 at 3:58 PM Tsukasa OI <research_trasio@irq.a4lg.com>
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.
> >     <https://github.com/a4lg/binutils-gdb/wiki/riscv_dis_arch_priv_spec>
> >
> >
> > 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:
> > <https://sourceware.org/pipermail/binutils/2023-July/128715.html>,
> > 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
>