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

[Palmer Dabbelt <palmer@dabbelt.com>]

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.

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