Re: [RFC] RISC-V: Add profile supports.

[Palmer Dabbelt <palmer@dabbelt.com>]

On Thu, 03 Nov 2022 12:11:31 PDT (-0700), christoph.muellner@vrull.eu wrote:
> On Wed, Nov 2, 2022 at 7:12 PM Palmer Dabbelt <palmer@dabbelt.com> wrote:
>>
>> On Wed, 02 Nov 2022 10:19:15 PDT (-0700), gcc-patches@gcc.gnu.org wrote:
>> > Could you add some test cases?
>>
>> Also documentation, and ideally some sort of spec for what this should
>> do so we can maintain compatibility with LLVM as well as we can.
>>
>> IIUC this also allows for profiles in the arch function attributes,
>> which would end up plumbing through to the assembler so we'd need
>> support there?  Probably best to just expand these out for the rest of
>> the tools so we don't need the profile->extension mappings everywhere,
>> IMO it's the same as the -mcpu discussion.
>>
>> >
>> > ---
>> >
>> > Parsing logic is kind of too adhoc, I would prefer using something
>> > like the following code to prevent magic pointer arithmetic like p+6:
>> >
>> > something like this:
>> >
>> > Table of all profile names = {"RVA20U64", riscv_profile::RVA20U64, ...}
>> >
>> > const char *rva20u64[] = {"m", "a", "f", "d",... NULL};
>> >
>> > table of profile content =
>> > {
>> >   {riscv_profile::RVA20U64, rva20u64},
>> >    ..
>> > }
>> >
>> > parse march ()
>> > {
>> >   if march is startswith
>> >   else if ((profile = parse_proile(march)) != risv_profile::NOT_PROFILE)
>> >      handle_profile (profile)
>> >   else
>> >      error
>> > }
>> >
>> > handle_profile (profile)
>> > {
>> >   use table of profile content to update ext.
>> > }
>> >
>> >
>> > On Wed, Nov 2, 2022 at 5:54 AM jiawei <jiawei@iscas.ac.cn> wrote:
>> >>handle_profile
>> >> Add two new function to handle profile input,
>> >> "parse_profile" will check if a input into -march is
>> >> legal, if it is then "handle_profile" will check the
>> >> profile's type[I/M/A], year[20/22] and mode[U/S/M],
>> >> set different extensions combine, just deal mandatory
>> >> part currently.
>> >>
>> >> gcc/ChangeLog:
>> >>
>> >>         * common/config/riscv/riscv-common.cc
>> >>         (riscv_subset_list::parse_profile): Check if profile name is valid or not.
>> >>         (riscv_subset_list::parse_std_ext): If input of -march option is
>> >>                                             a profile,skip first ISA check.
>> >>         (riscv_subset_list::parse): Handle rofile input in -march.
>> >>         (riscv_subset_list::handle_profile): Handle differen profiles
>> >>                                              expand to extensions.
>> >>         * config/riscv/riscv-subset.h: New function prototypes.
>> >>
>> >>
>> >> ---
>> >>  gcc/common/config/riscv/riscv-common.cc | 95 +++++++++++++++++++++++--
>> >>  gcc/config/riscv/riscv-subset.h         |  5 +-
>> >>  2 files changed, 94 insertions(+), 6 deletions(-)
>> >>
>> >> diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
>> >> index 602491c638d..da06bd89144 100644
>> >> --- a/gcc/common/config/riscv/riscv-common.cc
>> >> +++ b/gcc/common/config/riscv/riscv-common.cc
>> >> @@ -777,6 +777,35 @@ riscv_subset_list::parsing_subset_version (const char *ext,
>> >>    return p;
>> >>  }
>> >>
>> >> +/* Parsing function for profile.
>> >> +
>> >> +   Return Value:
>> >> +     Points to the end of profile.
>> >> +
>> >> +   Arguments:
>> >> +     `p`: Current parsing position.  */
>> >> +
>> >> +const char *
>> >> +riscv_subset_list::parse_profile (const char *p)
>> >> +{
>> >> +  if(*p == 'I' || *p == 'M' || *p == 'A'){
>> >> +    p++;
>> >> +    if(startswith (p, "20") || startswith (p, "22"))
>> >> +      p += 2;
>> >> +    if (*p == 'U' || *p == 'S' || *p == 'M')
>> >> +      p++;
>> >> +    if(startswith (p, "64") || startswith (p, "32")){
>> >> +       p += 2;
>> >> +       riscv_subset_list::handle_profile(p-6, p-4, p-3);
>> >> +       return p;
>> >> +    }
>> >> +  }
>> >> +  else
>> >> +    error_at (m_loc, "%<-march=%s%>: Invalid profile.", m_arch);
>> >> +  return NULL;
>> >> +}
>> >> +
>> >> +
>> >>  /* Parsing function for standard extensions.parse_std_ext
>> >>
>> >
>> > It's sort of too adhoc parsing the profile name, I would prefer using
>> > something like the following code to prevent magic pointer arithmetic
>> > like p+6.
>> > something
>> > Table of all profile names = {"RVA20U64", riscv_profile::RVA20U64, ...}
>> >
>> > const char *rva20u64[] = {"m", "a", "f", "d",... NULL};
>> >
>> > table of profile content =
>> > {
>> >   {riscv_profile::RVA20U64, rva20u64},
>> >    ..
>> > }
>> >
>> > parse march ()
>> > {
>> >   if march is startswith
>> >   else if ((profile = parse_proile(march)) != risv_profile::NOT_PROFILE)
>> >      handle_profile (profile)
>> >   else
>> >      error
>> > }
>> >
>> > handle_profile (profile)
>> > {
>> >   ad
>> > }
>> >
>> >>     Return Value:
>> >> @@ -786,7 +815,7 @@ riscv_subset_list::parsing_subset_version (const char *ext,
>> >>       `p`: Current parsing position.  */
>> >>
>> >>  const char *
>> >> -riscv_subset_list::parse_std_ext (const char *p)
>> >> +riscv_subset_list::parse_std_ext (const char *p, bool isprofile)
>> >>  {
>> >>    const char *all_std_exts = riscv_supported_std_ext ();
>> >>    const char *std_exts = all_std_exts;
>> >> @@ -795,8 +824,8 @@ riscv_subset_list::parse_std_ext (const char *p)
>> >>    unsigned minor_version = 0;
>> >>    char std_ext = '\0';
>> >>    bool explicit_version_p = false;
>> >> -
>> >> -  /* First letter must start with i, e or g.  */
>> >> +  if (!isprofile){
>> >> +    /* First letter must start with i, e or g.  */
>> >>    switch (*p)
>> >>      {
>> >>      case 'i':
>> >> @@ -850,6 +879,7 @@ riscv_subset_list::parse_std_ext (const char *p)
>> >>                 "%<i%> or %<g%>", m_arch);
>> >>        return NULL;
>> >>      }
>> >> +}
>> >>
>> >>    while (p != NULL && *p)
>> >>      {
>> >> @@ -1093,6 +1123,7 @@ riscv_subset_list::parse (const char *arch, location_t loc)
>> >>    riscv_subset_list *subset_list = new riscv_subset_list (arch, loc);
>> >>    riscv_subset_t *itr;
>> >>    const char *p = arch;
>> >> +  bool isprofile = false;
>> >>    if (startswith (p, "rv32"))
>> >>      {
>> >>        subset_list->m_xlen = 32;
>> >> @@ -1103,15 +1134,26 @@ riscv_subset_list::parse (const char *arch, location_t loc)
>> >>        subset_list->m_xlen = 64;
>> >>        p += 4;
>> >>      }
>> >> +  else if (startswith (p, "RV"))
>> >> +    {
>> >> +      if (startswith (p+6, "64"))
>> >> +       subset_list->m_xlen = 64;
>> >> +      else
>> >> +       subset_list->m_xlen = 32;
>> >> +      p += 2;
>> >> +      /* Parsing profile name.  */
>> >> +      p = subset_list->parse_profile (p);
>> >> +      isprofile = true;handle_profile
>> >> +    }
>> >>    else
>> >>      {
>> >> -      error_at (loc, "%<-march=%s%>: ISA string must begin with rv32 or rv64",
>> >> +      error_at (loc, "%<-march=%s%>: ISA string must begin with rv32 , rv64 or a profile",
>> >>                 arch);
>>
>> IMO we really don't want profiles in -march.  Unless I'm missing a
>> recent change, the profiles are a different namespace from the base ISAs
>> and trying to mix them into the same namespace is just going to lead to
>> headaches in the future.  We've got enough complexity with ISA strings
>> as it is.
>
> Thanks for pointing this out! A potential conflicting namespace is
> indeed a clear sign to not merge the ISA string and the profiles.
> A change to clarify the compatibility of the ISA string and the
> profiles has been suggested to the profiles spec and got accepted and
> merged with the words "Yes, this is guaranteed":
>   https://github.com/riscv/riscv-profiles/pull/78
>
> With this change in the profiles spec, we have the guarantee that the
> namespaces can be merged and thus there should be no namespace issue
> anymore with specifying the profile via -march.

As per the discussion on the RISC-V lists, that's not quite what we're 
looking for.  Sounds like the desired behavior here is actually to just 
not have -march take ISA strings.  That's fine with me, I just sent a 
patch to change the docs.

It's a pretty big policy change, though, so let's have the discussion 
over there to make sure it's more visible?

>
>>
>> >>        goto fail;
>> >>      }
>> >>
>> >>    /* Parsing standard extension.  */
>> >> -  p = subset_list->parse_std_ext (p);
>> >> +  p = subset_list->parse_std_ext (p,isprofile);
>> >>
>> >>    if (p == NULL)
>> >>      goto fail;
>> >> @@ -1349,6 +1391,49 @@ riscv_handle_option (struct gcc_options *opts,
>> >>      }
>> >>  }
>> >>
>> >> +/* Expand profile with defined mandatory extensions,
>> >> +   M-type/mode is emtpy and set as base right now.  */
>> >> +void riscv_subset_list::handle_profile(const char *profile_type,
>> >> +                       const char *profile_year,
>> >> +                       const char *profile_mode)
>> >> +{
>> >> +  add ("i", false);
>> >> +  if(*profile_type == 'A'){
>> >> +    add ("m", false);
>> >> +    add ("a", false);
>> >> +    add ("f", false);
>> >> +    add ("d", false);
>> >> +    add ("c", false);
>> >> +    add ("ziccamoa", false);
>> >> +    add ("ziccif", false);
>> >> +    add ("zicclsm", false);
>> >> +    add ("ziccrse", false);
>> >> +    add ("zicntr", false);
>> >> +    add ("zicsr", false);
>> >> +
>> >> +    if(*profile_mode == 'S')
>> >> +      add ("zifencei", false);
>>
>> Various other bits of the ISA specs say we're meant to ignore the
>> supervisor bits from compilers.  I'm not sure if that's true for the
>> profiles too?
>>
>> Though Zifencei is a doubly-complicated case, as it's forbidden by the
>> Linux uABI...
>>
>> >> +
>> >> +    if(*profile_year == '2')
>>
>> I think Kito pointed that out before, but the parsing here is pretty
>> ad-hoc.  This one looks buggy, though: it's treating all 2* profiles the
>> same, despite 20 being different from 22 and things like 21 and 23 being
>> undefined.
>>
>> >> +    {
>> >> +      add ("zihintpause", false);
>> >> +      add ("zihpm", false);
>> >> +      add ("zba", false);
>> >> +      add ("zbb", false);
>> >> +      add ("zbs", false);
>> >> +      add ("zicbom", false);
>> >> +      add ("zicbop", false);
>> >> +      add ("zicboz", false);
>> >> +      add ("zfhmin", false);
>> >> +      add ("zkt", false);
>> >> +      if(*profile_mode == 'S'){
>> >> +       add ("svpbmt", false);
>> >> +       add ("svinval", false);
>> >> +      }
>> >> +    }
>> >> +  }
>> >> +}
>>
>> Looks like there's nothing here that handles the optional, unsupported,
>> and unmentioned extensions?  It's not super clear what we should do with
>> those, but whatever it is we should document it so users aren't
>> surprised.
>>
>> This also doesn't cover the ISA spec versioning, which as far as I can
>> tell profiles don't directly mention but implicitly depend on (things
>> like having a mandatory fence.tso, for example).
>>
>> >> +
>> >>  /* Expand arch string with implied extensions.  */
>> >>
>> >>  const char *
>> >> diff --git a/gcc/config/riscv/riscv-subset.h b/gcc/config/riscv/riscv-subset.h
>> >> index 0bb3a9d29d0..303be5ed9ed 100644
>> >> --- a/gcc/config/riscv/riscv-subset.h
>> >> +++ b/gcc/config/riscv/riscv-subset.h
>> >> @@ -62,13 +62,16 @@ private:
>> >>    const char *parsing_subset_version (const char *, const char *, unsigned *,
>> >>                                       unsigned *, bool, bool *);
>> >>
>> >> -  const char *parse_std_ext (const char *);
>> >> +  const char *parse_profile (const char *);
>> >> +
>> >> +  const char *parse_std_ext (const char *, bool);
>> >>
>> >>    const char *parse_multiletter_ext (const char *, const char *,
>> >>                                      const char *);
>> >>
>> >>    void handle_implied_ext (riscv_subset_t *);
>> >>    void handle_combine_ext ();
>> >> +  void handle_profile(const char *, const char *, const char *);
>> >>
>> >>  public:
>> >>    ~riscv_subset_list ();
>> >> --
>> >> 2.25.1
>> >>