[RFC] RISC-V: Support RISC-V Profiles in -march option.

Message ID 20231120191447.2189928-1-jiawei@iscas.ac.cn
State Unresolved
Headers
Series [RFC] RISC-V: Support RISC-V Profiles in -march option. |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Jiawei Nov. 20, 2023, 7:14 p.m. UTC
  Supports RISC-V profiles[1] in -march option.

Default input set the profile is before other formal extensions.

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

gcc/ChangeLog:

        * common/config/riscv/riscv-common.cc (struct riscv_profiles):
          New struct.
        (riscv_subset_list::parse_profiles): New function.
        (riscv_subset_list::parse): New table.
        * config/riscv/riscv-subset.h: New protype.

gcc/testsuite/ChangeLog:

        * gcc.target/riscv/arch-29.c: New test.
        * gcc.target/riscv/arch-30.c: New test.
        * gcc.target/riscv/arch-31.c: New test.

---
 gcc/common/config/riscv/riscv-common.cc  | 58 +++++++++++++++++++++++-
 gcc/config/riscv/riscv-subset.h          |  2 +
 gcc/testsuite/gcc.target/riscv/arch-29.c |  5 ++
 gcc/testsuite/gcc.target/riscv/arch-30.c |  5 ++
 gcc/testsuite/gcc.target/riscv/arch-31.c |  5 ++
 6 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/arch-29.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/arch-30.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/arch-31.c
  

Comments

Jeff Law Dec. 11, 2023, 4:15 p.m. UTC | #1
On 11/20/23 12:14, Jiawei wrote:
> Supports RISC-V profiles[1] in -march option.
> 
> Default input set the profile is before other formal extensions.
> 
> [1]https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc
> 
> gcc/ChangeLog:
> 
>          * common/config/riscv/riscv-common.cc (struct riscv_profiles):
>            New struct.
>          (riscv_subset_list::parse_profiles): New function.
>          (riscv_subset_list::parse): New table.
>          * config/riscv/riscv-subset.h: New protype.
> 
> gcc/testsuite/ChangeLog:
> 
>          * gcc.target/riscv/arch-29.c: New test.
>          * gcc.target/riscv/arch-30.c: New test.
>          * gcc.target/riscv/arch-31.c: New test.
> 
> ---
>   gcc/common/config/riscv/riscv-common.cc  | 58 +++++++++++++++++++++++-
>   gcc/config/riscv/riscv-subset.h          |  2 +
>   gcc/testsuite/gcc.target/riscv/arch-29.c |  5 ++
>   gcc/testsuite/gcc.target/riscv/arch-30.c |  5 ++
>   gcc/testsuite/gcc.target/riscv/arch-31.c |  5 ++
>   6 files changed, 81 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/gcc.target/riscv/arch-29.c
>   create mode 100644 gcc/testsuite/gcc.target/riscv/arch-30.c
>   create mode 100644 gcc/testsuite/gcc.target/riscv/arch-31.c
> 
> diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
> index 5111626157b..30617e619b1 100644
> --- a/gcc/common/config/riscv/riscv-common.cc
> +++ b/gcc/common/config/riscv/riscv-common.cc
> @@ -165,6 +165,12 @@ struct riscv_ext_version
>     int minor_version;
>   };
>   
> +struct riscv_profiles
> +{
> +  const char * profile_name;
> +  const char * profile_string;
> +};
Just a formatting nit, no space between the '*' and the field name.

> @@ -348,6 +354,28 @@ static const struct riscv_ext_version riscv_combine_info[] =
>     {NULL, ISA_SPEC_CLASS_NONE, 0, 0}
>   };
>   
> +static const riscv_profiles riscv_profiles_table[] =
> +{
> +  {"RVI20U64", "rv64i"},
> +  {"RVI20U32", "rv32i"},
> +  /*Currently we don't have zicntr,ziccif,ziccrse,ziccamoa,
> +    zicclsm,za128rs yet.  */
It is actually useful to note the extensions not included?  I don't 
think the profiles are supposed to change once ratified.

> +  {"RVA22U64", "rv64imafdc_zicsr_zihintpause_zba_zbb_zbs_"				\
Note the trailing "_", was that intentional?  None of the other entries 
have a trailing "_".


> @@ -927,6 +955,31 @@ riscv_subset_list::parsing_subset_version (const char *ext,
>     return p;
>   }
>   
> +const char *
> +riscv_subset_list::parse_profiles (const char * p){
> +  for (int i = 0; riscv_profiles_table[i].profile_name != NULL; ++i) {
> +    const char* match = strstr(p, riscv_profiles_table[i].profile_name);
> +    const char* plus_ext = strchr(p, '+');
> +    /* Find profile at the begin.  */
> +    if (match != NULL && match == p) {
> +      /* If there's no '+' sign, return the profile_string directly.  */
> +      if(!plus_ext)
> +	return riscv_profiles_table[i].profile_string;
> +      /* If there's a '+' sign, concatenate profiles with other ext.  */
> +      else {
> +	size_t arch_len = strlen(riscv_profiles_table[i].profile_string) +
> +		strlen(plus_ext);
> +	static char* result = new char[arch_len + 2];
> +	strcpy(result, riscv_profiles_table[i].profile_string);
> +	strcat(result, "_");
> +	strcat(result, plus_ext + 1); /* skip the '+'.  */
> +	return result;
> +      }
> +    }
> +  }
> +  return p;
> +}
This needs a function comment.

The open curly should always be on a line by itself which is going to 
require reindenting all this code.  Comments go on separate lines rather 
than appending them to an existing line.


I think the consensus in the Tuesday patchwork meeting was that while 
there are concerns about profiles, those concerns should prevent this 
patch from going forward.  So if you could fix the formatting problem as 
well as the trailing "_" issue noted above and repost, it would be 
appreciated.

Thanks,

Jeff
  
Jiawei Dec. 12, 2023, 12:15 p.m. UTC | #2
> -----原始邮件-----
&gt; 发件人: "Jeff Law" <jeffreyalaw@gmail.com>
&gt; 发送时间: 2023-12-12 00:15:44 (星期二)
&gt; 收件人: Jiawei <jiawei@iscas.ac.cn>, gcc-patches@gcc.gnu.org
&gt; 抄送: kito.cheng@sifive.com, palmer@dabbelt.com, christoph.muellner@vrull.eu
&gt; 主题: Re: [RFC] RISC-V: Support RISC-V Profiles in -march option.
&gt; 
&gt; 
&gt; 
&gt; On 11/20/23 12:14, Jiawei wrote:
&gt; &gt; Supports RISC-V profiles[1] in -march option.
&gt; &gt; 
&gt; &gt; Default input set the profile is before other formal extensions.
&gt; &gt; 
&gt; &gt; [1]https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc
&gt; &gt; 
&gt; &gt; gcc/ChangeLog:
&gt; &gt; 
&gt; &gt;          * common/config/riscv/riscv-common.cc (struct riscv_profiles):
&gt; &gt;            New struct.
&gt; &gt;          (riscv_subset_list::parse_profiles): New function.
&gt; &gt;          (riscv_subset_list::parse): New table.
&gt; &gt;          * config/riscv/riscv-subset.h: New protype.
&gt; &gt; 
&gt; &gt; gcc/testsuite/ChangeLog:
&gt; &gt; 
&gt; &gt;          * gcc.target/riscv/arch-29.c: New test.
&gt; &gt;          * gcc.target/riscv/arch-30.c: New test.
&gt; &gt;          * gcc.target/riscv/arch-31.c: New test.
&gt; &gt; 
&gt; &gt; ---
&gt; &gt;   gcc/common/config/riscv/riscv-common.cc  | 58 +++++++++++++++++++++++-
&gt; &gt;   gcc/config/riscv/riscv-subset.h          |  2 +
&gt; &gt;   gcc/testsuite/gcc.target/riscv/arch-29.c |  5 ++
&gt; &gt;   gcc/testsuite/gcc.target/riscv/arch-30.c |  5 ++
&gt; &gt;   gcc/testsuite/gcc.target/riscv/arch-31.c |  5 ++
&gt; &gt;   6 files changed, 81 insertions(+), 1 deletion(-)
&gt; &gt;   create mode 100644 gcc/testsuite/gcc.target/riscv/arch-29.c
&gt; &gt;   create mode 100644 gcc/testsuite/gcc.target/riscv/arch-30.c
&gt; &gt;   create mode 100644 gcc/testsuite/gcc.target/riscv/arch-31.c
&gt; &gt; 
&gt; &gt; diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
&gt; &gt; index 5111626157b..30617e619b1 100644
&gt; &gt; --- a/gcc/common/config/riscv/riscv-common.cc
&gt; &gt; +++ b/gcc/common/config/riscv/riscv-common.cc
&gt; &gt; @@ -165,6 +165,12 @@ struct riscv_ext_version
&gt; &gt;     int minor_version;
&gt; &gt;   };
&gt; &gt;   
&gt; &gt; +struct riscv_profiles
&gt; &gt; +{
&gt; &gt; +  const char * profile_name;
&gt; &gt; +  const char * profile_string;
&gt; &gt; +};
&gt; Just a formatting nit, no space between the '*' and the field name.

Fixed.

&gt; 
&gt; &gt; @@ -348,6 +354,28 @@ static const struct riscv_ext_version riscv_combine_info[] =
&gt; &gt;     {NULL, ISA_SPEC_CLASS_NONE, 0, 0}
&gt; &gt;   };
&gt; &gt;   
&gt; &gt; +static const riscv_profiles riscv_profiles_table[] =
&gt; &gt; +{
&gt; &gt; +  {"RVI20U64", "rv64i"},
&gt; &gt; +  {"RVI20U32", "rv32i"},
&gt; &gt; +  /*Currently we don't have zicntr,ziccif,ziccrse,ziccamoa,
&gt; &gt; +    zicclsm,za128rs yet.  */
&gt; It is actually useful to note the extensions not included?  I don't 
&gt; think the profiles are supposed to change once ratified.
&gt; 
&gt; &gt; +  {"RVA22U64", "rv64imafdc_zicsr_zihintpause_zba_zbb_zbs_"				\
&gt; Note the trailing "_", was that intentional?  None of the other entries 
&gt; have a trailing "_".

Here is a line break due to too long length of arch string,
Adjusted the format in the new patch.

&gt; 
&gt; 
&gt; &gt; @@ -927,6 +955,31 @@ riscv_subset_list::parsing_subset_version (const char *ext,
&gt; &gt;     return p;
&gt; &gt;   }
&gt; &gt;   
&gt; &gt; +const char *
&gt; &gt; +riscv_subset_list::parse_profiles (const char * p){
&gt; &gt; +  for (int i = 0; riscv_profiles_table[i].profile_name != NULL; ++i) {
&gt; &gt; +    const char* match = strstr(p, riscv_profiles_table[i].profile_name);
&gt; &gt; +    const char* plus_ext = strchr(p, '+');
&gt; &gt; +    /* Find profile at the begin.  */
&gt; &gt; +    if (match != NULL &amp;&amp; match == p) {
&gt; &gt; +      /* If there's no '+' sign, return the profile_string directly.  */
&gt; &gt; +      if(!plus_ext)
&gt; &gt; +	return riscv_profiles_table[i].profile_string;
&gt; &gt; +      /* If there's a '+' sign, concatenate profiles with other ext.  */
&gt; &gt; +      else {
&gt; &gt; +	size_t arch_len = strlen(riscv_profiles_table[i].profile_string) +
&gt; &gt; +		strlen(plus_ext);
&gt; &gt; +	static char* result = new char[arch_len + 2];
&gt; &gt; +	strcpy(result, riscv_profiles_table[i].profile_string);
&gt; &gt; +	strcat(result, "_");
&gt; &gt; +	strcat(result, plus_ext + 1); /* skip the '+'.  */
&gt; &gt; +	return result;
&gt; &gt; +      }
&gt; &gt; +    }
&gt; &gt; +  }
&gt; &gt; +  return p;
&gt; &gt; +}
&gt; This needs a function comment.

Thanks, added the parse function descrption and some deal logical.

&gt; 
&gt; The open curly should always be on a line by itself which is going to 
&gt; require reindenting all this code.  Comments go on separate lines rather 
&gt; than appending them to an existing line.
&gt; 
&gt; 
&gt; I think the consensus in the Tuesday patchwork meeting was that while 
&gt; there are concerns about profiles, those concerns should prevent this 
&gt; patch from going forward.  So if you could fix the formatting problem as 
&gt; well as the trailing "_" issue noted above and repost, it would be 
&gt; appreciated.
&gt; 
&gt; Thanks,
&gt; 
&gt; Jeff

Thanks for your review and comments, I had update them in the new patch:

https://gcc.gnu.org/pipermail/gcc-patches/2023-December/640324.html

BR,
Jiawei</jiawei@iscas.ac.cn></jeffreyalaw@gmail.com>
  

Patch

diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
index 5111626157b..30617e619b1 100644
--- a/gcc/common/config/riscv/riscv-common.cc
+++ b/gcc/common/config/riscv/riscv-common.cc
@@ -165,6 +165,12 @@  struct riscv_ext_version
   int minor_version;
 };
 
+struct riscv_profiles
+{
+  const char * profile_name;
+  const char * profile_string;
+};
+
 /* All standard extensions defined in all supported ISA spec.  */
 static const struct riscv_ext_version riscv_ext_version_table[] =
 {
@@ -348,6 +354,28 @@  static const struct riscv_ext_version riscv_combine_info[] =
   {NULL, ISA_SPEC_CLASS_NONE, 0, 0}
 };
 
+static const riscv_profiles riscv_profiles_table[] =
+{
+  {"RVI20U64", "rv64i"},
+  {"RVI20U32", "rv32i"},
+  /*Currently we don't have zicntr,ziccif,ziccrse,ziccamoa,
+    zicclsm,za128rs yet.  */
+  {"RVA20U64", "rv64imafdc_zicsr"},
+  /*Ss1p11, svbare, sv39, svade, sscptr, ssvecd, sstvala should
+    control by binutils.  */
+  {"RVA20S64", "rv64imafdc_zicsr_zifencei"},
+  /*Currently we don't have zicntr,zihpm,ziccif,ziccrse,ziccamoa,
+    zicclsm,zic64b,za64rs yet.  */
+  {"RVA22U64", "rv64imafdc_zicsr_zihintpause_zba_zbb_zbs_"				\
+   "zicbom_zicbop_zicboz_zfhmin_zkt"},
+  /*Ss1p12, svbare, sv39, svade, sscptr, ssvecd, sstvala,
+    scounterenw should control by binutils.  */
+  {"RVA22S64","rv64imafdc_zicsr_zifencei_zihintpause"				\
+   "_zba_zbb_zbs_zicbom_zicbop_zicboz_zfhmin_zkt_svpbmt_svinval"},
+  /* Terminate the list.  */
+  {NULL, NULL}
+};
+
 static const riscv_cpu_info riscv_cpu_tables[] =
 {
 #define RISCV_CORE(CORE_NAME, ARCH, TUNE) \
@@ -927,6 +955,31 @@  riscv_subset_list::parsing_subset_version (const char *ext,
   return p;
 }
 
+const char *
+riscv_subset_list::parse_profiles (const char * p){
+  for (int i = 0; riscv_profiles_table[i].profile_name != NULL; ++i) {
+    const char* match = strstr(p, riscv_profiles_table[i].profile_name);
+    const char* plus_ext = strchr(p, '+');
+    /* Find profile at the begin.  */
+    if (match != NULL && match == p) {
+      /* If there's no '+' sign, return the profile_string directly.  */
+      if(!plus_ext)
+	return riscv_profiles_table[i].profile_string;
+      /* If there's a '+' sign, concatenate profiles with other ext.  */
+      else {
+	size_t arch_len = strlen(riscv_profiles_table[i].profile_string) +
+		strlen(plus_ext);
+	static char* result = new char[arch_len + 2];
+	strcpy(result, riscv_profiles_table[i].profile_string);
+	strcat(result, "_");
+	strcat(result, plus_ext + 1); /* skip the '+'.  */
+	return result;
+      }
+    }
+  }
+  return p;
+}
+
 /* Parsing function for standard extensions.
 
    Return Value:
@@ -1430,7 +1483,10 @@  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;
+  p = subset_list->parse_profiles(p);
+
   if (startswith (p, "rv32"))
     {
       subset_list->m_xlen = 32;
@@ -1443,7 +1499,7 @@  riscv_subset_list::parse (const char *arch, location_t loc)
     }
   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;
     }
diff --git a/gcc/config/riscv/riscv-subset.h b/gcc/config/riscv/riscv-subset.h
index d2a4bd20530..c8b778330b4 100644
--- a/gcc/config/riscv/riscv-subset.h
+++ b/gcc/config/riscv/riscv-subset.h
@@ -76,6 +76,8 @@  private:
   const char *parse_single_multiletter_ext (const char *, const char *,
 					    const char *);
 
+  const char *parse_profiles (const char*);
+
   void handle_implied_ext (const char *);
   bool check_implied_ext ();
   void handle_combine_ext ();
diff --git a/gcc/testsuite/gcc.target/riscv/arch-29.c b/gcc/testsuite/gcc.target/riscv/arch-29.c
new file mode 100644
index 00000000000..eb24abe4153
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/arch-29.c
@@ -0,0 +1,5 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=RVI20U64 -mabi=lp64" } */
+int foo()
+{
+}
diff --git a/gcc/testsuite/gcc.target/riscv/arch-30.c b/gcc/testsuite/gcc.target/riscv/arch-30.c
new file mode 100644
index 00000000000..bc556a3e717
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/arch-30.c
@@ -0,0 +1,5 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=RVI20U64+mafdc -mabi=lp64d" } */
+int foo()
+{
+}
diff --git a/gcc/testsuite/gcc.target/riscv/arch-31.c b/gcc/testsuite/gcc.target/riscv/arch-31.c
new file mode 100644
index 00000000000..a7b80a5ad43
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/arch-31.c
@@ -0,0 +1,5 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=RVA22S64 -mabi=lp64d" } */
+int foo()
+{
+}