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

[Palmer Dabbelt <palmer@dabbelt.com>]

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.

> +  {"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)