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

[Tsukasa OI <research_trasio@irq.a4lg.com>]

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 <research_trasio@irq.a4lg.com
> <mailto: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
>     <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
>     <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
>