Re: [RFC] RISC-V: Minimal supports for new extensions in profile.

[Philipp Tomsich <philipp.tomsich@vrull.eu>]

On Wed, 2 Nov 2022 at 23:06, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Wed, 02 Nov 2022 05:52:34 PDT (-0700), jiawei@iscas.ac.cn wrote:
> > This patch just add name support contain in profiles.
> > Set the extension version as 0.1.
>
> Or maybe v0.8, as they're in the v0.8 profile spec?  I doubt it really
> matters, though.  Either way we'll need a -mprofile-spec-version (or
> whatever) for these, as these one-phrase definitions will almost
> certainly change.
>
> This also doesn't couple these new extensions to the profiles in any
> way.  IMO that's a sane thing to do, but they're only defined as part of
> the mandatory profile section so I'm just double-checking here
> <https://github.com/riscv/riscv-profiles/issues/77>.
>
> We'll also need news entries and I don't see any testing results, though
> those are probably pretty easy here.
>
> >
> > gcc/ChangeLog:
> >
> >         * common/config/riscv/riscv-common.cc: New extensions.
> >         * config/riscv/riscv-opts.h (MASK_ZICCAMOA): New mask.
> >         (MASK_ZICCIF): Ditto.
> >         (MASK_ZICCLSM): Ditto.
> >         (MASK_ZICCRSE): Ditto.
> >         (MASK_ZICNTR): Ditto.
> >         (MASK_ZIHINTPAUSE): Ditto.
> >         (MASK_ZIHPM): Ditto.
> >         (TARGET_ZICCAMOA): New target.
> >         (TARGET_ZICCIF): Ditto.
> >         (TARGET_ZICCLSM): Ditto.
> >         (TARGET_ZICCRSE): Ditto.
> >         (TARGET_ZICNTR): Ditto.
> >         (TARGET_ZIHINTPAUSE): Ditto.
> >         (TARGET_ZIHPM): Ditto.
> >         (MASK_SVPBMT): New mask.
> >
> > ---
> >  gcc/common/config/riscv/riscv-common.cc | 20 ++++++++++++++++++++
> >  gcc/config/riscv/riscv-opts.h           | 15 +++++++++++++++
> >  2 files changed, 35 insertions(+)
> >
> > diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
> > index d6404a01205..602491c638d 100644
> > --- a/gcc/common/config/riscv/riscv-common.cc
> > +++ b/gcc/common/config/riscv/riscv-common.cc
> > @@ -163,6 +163,15 @@ static const struct riscv_ext_version riscv_ext_version_table[] =
> >    {"zifencei", ISA_SPEC_CLASS_20191213, 2, 0},
> >    {"zifencei", ISA_SPEC_CLASS_20190608, 2, 0},
> >
> > +  {"ziccamoa", ISA_SPEC_CLASS_NONE, 0, 1},
> > +  {"ziccif", ISA_SPEC_CLASS_NONE, 0, 1},
>
> IMO Ziccif should be sufficiently visible in the object that we can
> reject running binaries that require that on systems that don't support
> it.  It's essentially the same as Ztso, we're adding more constraints to
> existing instructions.
>
> > +  {"zicclsm", ISA_SPEC_CLASS_NONE, 0, 1},
> > +  {"ziccrse", ISA_SPEC_CLASS_NONE, 0, 1},
> > +  {"zicntr", ISA_SPEC_CLASS_NONE, 0, 1},
>
> As per Andrew's post here
> <https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/QKjQhChrq9Q/m/7gqdkctgAgAJ>,
> Zicntr and Zihpm should be ignored by software.
>
> I think you could make that compatibility argument for Zicclsm and
> Ziccrse as well, but given that the core of the Zicntr/Zihpm argument is
> based on userspace not knowing about priv-spec details such as PMAs I'm
> guessing it'd go that way too.  That said, these are all listed in the
> "features available to user-mode execution environments" section.
>
> > +
> > +  {"zihintpause", ISA_SPEC_CLASS_NONE, 0, 1},
>
> We should probably have a builtin for this, there's a handful of
> userspace cpu_relax()-type calls and having something to select the
> flavor of pause instruction based on the target seems generally useful.

I had originally submitted this in early 2021 (including a builtin),
but we never agreed on details (e.g. whether this should be gated, as
it is a true hint):
  https://gcc.gnu.org/pipermail/gcc-patches/2021-January/562936.html

Let me know what behavior we want and I'll submit a v2.

Philipp.

> > +  {"zihpm", ISA_SPEC_CLASS_NONE, 0, 1},
>
> See above.
>
> > +
> >    {"zba", ISA_SPEC_CLASS_NONE, 1, 0},
> >    {"zbb", ISA_SPEC_CLASS_NONE, 1, 0},
> >    {"zbc", ISA_SPEC_CLASS_NONE, 1, 0},
>
> There's some missing ones, just poking through the profile I can find:
> Za64rs and Zic64b, but there's a lot in there and I'm kind of getting my
> eyes crossed already.
>
> I'd argue that Za64rs should be handled like Ziccif, but we don't have a
> lot of bits left in the header.  I just sent some patches to the ELF
> psABI spec: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/pull/351
>
> > @@ -219,6 +228,7 @@ static const struct riscv_ext_version riscv_ext_version_table[] =
> >
> >    {"svinval", ISA_SPEC_CLASS_NONE, 1, 0},
> >    {"svnapot", ISA_SPEC_CLASS_NONE, 1, 0},
> > +  {"svpbmt", ISA_SPEC_CLASS_NONE, 0, 1},
> >
> >    /* Terminate the list.  */
> >    {NULL, ISA_SPEC_CLASS_NONE, 0, 0}
> > @@ -1179,6 +1189,14 @@ static const riscv_ext_flag_table_t riscv_ext_flag_table[] =
> >
> >    {"zicsr",    &gcc_options::x_riscv_zi_subext, MASK_ZICSR},
> >    {"zifencei", &gcc_options::x_riscv_zi_subext, MASK_ZIFENCEI},
> > +  {"ziccamoa", &gcc_options::x_riscv_zi_subext, MASK_ZICCAMOA},
> > +  {"ziccif", &gcc_options::x_riscv_zi_subext, MASK_ZICCIF},
> > +  {"zicclsm", &gcc_options::x_riscv_zi_subext, MASK_ZICCLSM},
> > +  {"ziccrse", &gcc_options::x_riscv_zi_subext, MASK_ZICCRSE},
> > +  {"zicntr", &gcc_options::x_riscv_zi_subext, MASK_ZICNTR},
> > +
> > +  {"zihintpause", &gcc_options::x_riscv_zi_subext, MASK_ZIHINTPAUSE},
> > +  {"zihpm", &gcc_options::x_riscv_zi_subext, MASK_ZIHPM},
> >
> >    {"zba",    &gcc_options::x_riscv_zb_subext, MASK_ZBA},
> >    {"zbb",    &gcc_options::x_riscv_zb_subext, MASK_ZBB},
> > @@ -1230,6 +1248,7 @@ static const riscv_ext_flag_table_t riscv_ext_flag_table[] =
> >    {"zvl1024b",  &gcc_options::x_riscv_zvl_flags, MASK_ZVL1024B},
> >    {"zvl2048b",  &gcc_options::x_riscv_zvl_flags, MASK_ZVL2048B},
> >    {"zvl4096b",  &gcc_options::x_riscv_zvl_flags, MASK_ZVL4096B},
> > +
>
> Looks like a spurious newline got inserted?  If there's a reason for it
> then it's best to do this as its own commit.
>
> >    {"zvl8192b",  &gcc_options::x_riscv_zvl_flags, MASK_ZVL8192B},
> >    {"zvl16384b", &gcc_options::x_riscv_zvl_flags, MASK_ZVL16384B},
> >    {"zvl32768b", &gcc_options::x_riscv_zvl_flags, MASK_ZVL32768B},
> > @@ -1242,6 +1261,7 @@ static const riscv_ext_flag_table_t riscv_ext_flag_table[] =
> >
> >    {"svinval", &gcc_options::x_riscv_sv_subext, MASK_SVINVAL},
> >    {"svnapot", &gcc_options::x_riscv_sv_subext, MASK_SVNAPOT},
> > +  {"svpbmt", &gcc_options::x_riscv_sv_subext, MASK_SVPBMT},
>
> Not technically part of the profiles so we could split it out, but I
> don't think it's strictly necessary.
>
> >    {NULL, NULL, 0}
> >  };
> > diff --git a/gcc/config/riscv/riscv-opts.h b/gcc/config/riscv/riscv-opts.h
> > index 1dfe8c89209..906b6280188 100644
> > --- a/gcc/config/riscv/riscv-opts.h
> > +++ b/gcc/config/riscv/riscv-opts.h
> > @@ -69,9 +69,23 @@ enum stack_protector_guard {
> >
> >  #define MASK_ZICSR    (1 << 0)
> >  #define MASK_ZIFENCEI (1 << 1)
> > +#define MASK_ZICCAMOA (1 << 2)
> > +#define MASK_ZICCIF   (1 << 3)
> > +#define MASK_ZICCLSM  (1 << 4)
> > +#define MASK_ZICCRSE  (1 << 5)
> > +#define MASK_ZICNTR   (1 << 6)
> > +#define MASK_ZIHINTPAUSE (1 << 7)
> > +#define MASK_ZIHPM    (1 << 8)
> >
> >  #define TARGET_ZICSR    ((riscv_zi_subext & MASK_ZICSR) != 0)
> >  #define TARGET_ZIFENCEI ((riscv_zi_subext & MASK_ZIFENCEI) != 0)
> > +#define TARGET_ZICCAMOA ((riscv_zi_subext & MASK_ZICCAMOA) != 0)
> > +#define TARGET_ZICCIF   ((riscv_zi_subext & MASK_ZICCIF) != 0)
> > +#define TARGET_ZICCLSM  ((riscv_zi_subext & MASK_ZICCLSM) != 0)
> > +#define TARGET_ZICCRSE  ((riscv_zi_subext & MASK_ZICCRSE) != 0)
> > +#define TARGET_ZICNTR   ((riscv_zi_subext & MASK_ZICNTR) != 0)
> > +#define TARGET_ZIHINTPAUSE ((riscv_zi_subext & MASK_ZIHINTPAUSE) != 0)
> > +#define TARGET_ZIHPM    ((riscv_zi_subext & MASK_ZIHPM) != 0)
> >
> >  #define MASK_ZBA      (1 << 0)
> >  #define MASK_ZBB      (1 << 1)
> > @@ -174,6 +188,7 @@ enum stack_protector_guard {
> >
> >  #define MASK_SVINVAL (1 << 0)
> >  #define MASK_SVNAPOT (1 << 1)
> > +#define MASK_SVPBMT   (1 << 2)
> >
> >  #define TARGET_SVINVAL ((riscv_sv_subext & MASK_SVINVAL) != 0)
> >  #define TARGET_SVNAPOT ((riscv_sv_subext & MASK_SVNAPOT) != 0)