[RFC] RISC-V: Add profile supports.

[RFC] RISC-V: Add profile supports.

[jiawei]

Supports RISC-V profiles[1] in -march option, add minimal extension name supports.

Default input set the profile is before other formal extensions.

Test with -march=RV[I/M/A]2[0/2][U/M/S][64/32]+otherextensions.

[1]https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc


jiawei (2):
  RISC-V: Add minimal supports for new extension in profile.
  RISC-V: Add profile supports.

 gcc/common/config/riscv/riscv-common.cc | 115 ++++++++++++++++++++++--
 gcc/config/riscv/riscv-opts.h           |  15 ++++
 gcc/config/riscv/riscv-subset.h         |   5 +-
 3 files changed, 129 insertions(+), 6 deletions(-)

-- 
2.25.1

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

[jiawei]

This patch just add name support contain in profiles.
Set the extension version as 0.1.

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},
+  {"zicclsm", ISA_SPEC_CLASS_NONE, 0, 1},
+  {"ziccrse", ISA_SPEC_CLASS_NONE, 0, 1},
+  {"zicntr", ISA_SPEC_CLASS_NONE, 0, 1},
+
+  {"zihintpause", ISA_SPEC_CLASS_NONE, 0, 1},
+  {"zihpm", ISA_SPEC_CLASS_NONE, 0, 1},
+
   {"zba", ISA_SPEC_CLASS_NONE, 1, 0},
   {"zbb", ISA_SPEC_CLASS_NONE, 1, 0},
   {"zbc", ISA_SPEC_CLASS_NONE, 1, 0},
@@ -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},
+
   {"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},
 
   {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)
-- 
2.25.1

[RFC] RISC-V: Add profile supports.

[jiawei]

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.
 
    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;
+    }
   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);
       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);
+  
+    if(*profile_year == '2')
+    {
+      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);
+      }
+    }
+  }
+}
+
 /* 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

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

[Kito Cheng]

Could you add some test cases?

---

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);
>        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);
> +
> +    if(*profile_year == '2')
> +    {
> +      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);
> +      }
> +    }
> +  }
> +}
> +
>  /* 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
>

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

[Palmer Dabbelt]

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

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

[Palmer Dabbelt]

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)

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

[Philipp Tomsich]

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)

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

[Palmer Dabbelt]

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)

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

[Christoph Müllner]

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.

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

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

[Palmer Dabbelt]

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