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

[Palmer Dabbelt <palmer@dabbelt.com>]

On Wed, 02 Nov 2022 15:20:57 PDT (-0700), philipp.tomsich@vrull.eu wrote:
> 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.

Ah, sorry, I guess I forgot.  I don't know if we ever talked about it in 
GCC land, but at least in QEMU and Linux we ended up just ignoring the 
ISA manual here and pretending it's a hint -- the assumption is that 
vendors will do so too.  So IMO we can just document that somewhere in 
GCC as well.

I guess we could add some sort of Xsifive_x0div_relax extension to cover 
the div-based go-slow instructions in some SiFive machines as well, but 
that's sort of a different discussion.

> 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)