diff mbox series

RISC-V: Preserve arch version info during normalizing arch string

Message ID 20200630020049.107362-1-kito.cheng@sifive.com
State New
Headers show
Series RISC-V: Preserve arch version info during normalizing arch string | expand

Commit Message

Kito Cheng June 30, 2020, 2 a.m. UTC 
- Arch version should preserved if user explicitly specified the version.
  e.g.
    After normalize, -march=rv32if3d should be -march=rv32i_f3p0d
    instead of-march=rv32ifd.

gcc/ChangeLog:

	* common/config/riscv/riscv-common.c(riscv_subset_t): New field
	added.
	(riscv_subset_list::parsing_subset_version): Add parameter for
	indicate explicitly version, and handle explicitly version.
	(riscv_subset_list::handle_implied_ext): Ditto.
	(riscv_subset_list::add): Ditto.
	(riscv_subset_t::riscv_subset_t): Init new field.
	(riscv_subset_list::to_string): Always output version info if version
	explicitly specified.
	(riscv_subset_list::parsing_subset_version): Handle explicitly
	arch version.
	(riscv_subset_list::parse_std_ext): Ditto.
	(riscv_subset_list::parse_multiletter_ext): Ditto.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/attribute-13.c: New.
---
 gcc/common/config/riscv/riscv-common.c        | 70 ++++++++++++-------
 gcc/testsuite/gcc.target/riscv/attribute-13.c |  6 ++
 2 files changed, 52 insertions(+), 24 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/attribute-13.c

Comments

Jim Wilson June 30, 2020, 11:02 p.m. UTC | #1 
On Mon, Jun 29, 2020 at 7:00 PM Kito Cheng <kito.cheng@sifive.com> wrote:
> - Arch version should preserved if user explicitly specified the version.
>   e.g.
>     After normalize, -march=rv32if3d should be -march=rv32i_f3p0d
>     instead of-march=rv32ifd.

This looks good to me.

> +    explicit_version_p(false)

I'd suggest a space before the open paren.

This isn't a criticism of your patch, but I noticed while looking at
this that we accept gXpY and expand to iXpY_mXpY_ etc.  However, imafd
are all numbered independently.  It doesn't make much sense to specify
a version with g and assume it is correct for all of imafd.  Similarly
with the implied extensions, e.g. dXpY is expanded to fXpY_dXpY,
though in this case it is likely that f and d numbers will remain
compatible.  And q too.  Still it looks a bit funny to be making that
assumption and there probably will be other examples where the
versions of the implied extensions won't match the specified
extension.  Actually I see that f implies Zicsr, and those two have
different version numbers, we just haven't implemented Zicsr yet.  We
are correctly implementing the rules as specified, the rules just
don't make sense here.  We probably need to file an issue against the
riscv-isa-manual project for a clarification of how to handle this
stuff.

Jim
Kito Cheng July 1, 2020, 3:16 a.m. UTC | #2 
Hi Jim:

> This isn't a criticism of your patch, but I noticed while looking at
> this that we accept gXpY and expand to iXpY_mXpY_ etc.  However, imafd
> are all numbered independently.  It doesn't make much sense to specify
> a version with g and assume it is correct for all of imafd.  Similarly
> with the implied extensions, e.g. dXpY is expanded to fXpY_dXpY,
> though in this case it is likely that f and d numbers will remain
> compatible.  And q too.  Still it looks a bit funny to be making that
> assumption and there probably will be other examples where the
> versions of the implied extensions won't match the specified
> extension.  Actually I see that f implies Zicsr, and those two have
> different version numbers, we just haven't implemented Zicsr yet.  We
> are correctly implementing the rules as specified, the rules just
> don't make sense here.  We probably need to file an issue against the
> riscv-isa-manual project for a clarification of how to handle this
> stuff.

I agree the version of G is kind of problematic for GCC implementation,
That reminds me there was a long discussion[1] last year,
The conclusion is version of G is too confusing, it might just don't
accept any version for G.
I thought it could deprecate the version for G in GCC 11 and then drop
that in GCC 12?
For riscv-isa-manual, we could ask them to add a note about the G
don't accept version?
What do you think about this?

And the -misa-spec is supported on the binutils side, so I guess it's
time to start to improve
those things on GCC now.

[1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/aZhMG7NIVTk/m/hX2BRWsMEQAJ

Thanks :)

>
> Jim
Kito Cheng July 1, 2020, 3:34 a.m. UTC | #3 
Committed with coding style fixing.

On Wed, Jul 1, 2020 at 11:16 AM Kito Cheng <kito.cheng@sifive.com> wrote:
>
> Hi Jim:
>
> > This isn't a criticism of your patch, but I noticed while looking at
> > this that we accept gXpY and expand to iXpY_mXpY_ etc.  However, imafd
> > are all numbered independently.  It doesn't make much sense to specify
> > a version with g and assume it is correct for all of imafd.  Similarly
> > with the implied extensions, e.g. dXpY is expanded to fXpY_dXpY,
> > though in this case it is likely that f and d numbers will remain
> > compatible.  And q too.  Still it looks a bit funny to be making that
> > assumption and there probably will be other examples where the
> > versions of the implied extensions won't match the specified
> > extension.  Actually I see that f implies Zicsr, and those two have
> > different version numbers, we just haven't implemented Zicsr yet.  We
> > are correctly implementing the rules as specified, the rules just
> > don't make sense here.  We probably need to file an issue against the
> > riscv-isa-manual project for a clarification of how to handle this
> > stuff.
>
> I agree the version of G is kind of problematic for GCC implementation,
> That reminds me there was a long discussion[1] last year,
> The conclusion is version of G is too confusing, it might just don't
> accept any version for G.
> I thought it could deprecate the version for G in GCC 11 and then drop
> that in GCC 12?
> For riscv-isa-manual, we could ask them to add a note about the G
> don't accept version?
> What do you think about this?
>
> And the -misa-spec is supported on the binutils side, so I guess it's
> time to start to improve
> those things on GCC now.
>
> [1] https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/aZhMG7NIVTk/m/hX2BRWsMEQAJ
>
> Thanks :)
>
> >
> > Jim
Jim Wilson July 1, 2020, 11:32 p.m. UTC | #4 
On Tue, Jun 30, 2020 at 8:16 PM Kito Cheng <kito.cheng@sifive.com> wrote:
> I agree the version of G is kind of problematic for GCC implementation,
> That reminds me there was a long discussion[1] last year,
> The conclusion is version of G is too confusing, it might just don't
> accept any version for G.
> I thought it could deprecate the version for G in GCC 11 and then drop
> that in GCC 12?
> For riscv-isa-manual, we could ask them to add a note about the G
> don't accept version?
> What do you think about this?

I think it is reasonable to start a discussion in riscv-isa-manual, to
try to get an official answer for what is valid and how to interpret
it, instead of just making up our own rules.

Jim
Kito Cheng July 2, 2020, 3 a.m. UTC | #5 
Hi Jim:
> I think it is reasonable to start a discussion in riscv-isa-manual, to
> try to get an official answer for what is valid and how to interpret
> it, instead of just making up our own rules.

Agree, issue[1] created on riscv-isa-manual.

[1] https://github.com/riscv/riscv-isa-manual/issues/533
diff mbox series

Patch

diff --git a/gcc/common/config/riscv/riscv-common.c b/gcc/common/config/riscv/riscv-common.c
index 2df93460165b..96a128acdeac 100644
--- a/gcc/common/config/riscv/riscv-common.c
+++ b/gcc/common/config/riscv/riscv-common.c
@@ -42,6 +42,8 @@  struct riscv_subset_t
   int major_version;
   int minor_version;
   struct riscv_subset_t *next;
+
+  bool explicit_version_p;
 };
 
 /* Type for implied ISA info.  */
@@ -80,19 +82,19 @@  private:
   riscv_subset_list (const char *, location_t);
 
   const char *parsing_subset_version (const char *, unsigned *, unsigned *,
-				      unsigned, unsigned, bool);
+				      unsigned, unsigned, bool, bool *);
 
   const char *parse_std_ext (const char *);
 
   const char *parse_multiletter_ext (const char *, const char *,
 				     const char *);
 
-  void handle_implied_ext (const char *, int, int);
+  void handle_implied_ext (const char *, int, int, bool);
 
 public:
   ~riscv_subset_list ();
 
-  void add (const char *, int, int);
+  void add (const char *, int, int, bool);
 
   riscv_subset_t *lookup (const char *,
 			  int major_version = RISCV_DONT_CARE_VERSION,
@@ -111,7 +113,8 @@  static const char *riscv_supported_std_ext (void);
 static riscv_subset_list *current_subset_list = NULL;
 
 riscv_subset_t::riscv_subset_t ()
-  : name (), major_version (0), minor_version (0), next (NULL)
+  : name (), major_version (0), minor_version (0), next (NULL),
+    explicit_version_p(false)
 {
 }
 
@@ -138,7 +141,7 @@  riscv_subset_list::~riscv_subset_list ()
 
 void
 riscv_subset_list::add (const char *subset, int major_version,
-			int minor_version)
+			int minor_version, bool explicit_version_p)
 {
   riscv_subset_t *s = new riscv_subset_t ();
 
@@ -148,6 +151,7 @@  riscv_subset_list::add (const char *subset, int major_version,
   s->name = subset;
   s->major_version = major_version;
   s->minor_version = minor_version;
+  s->explicit_version_p = explicit_version_p;
   s->next = NULL;
 
   if (m_tail != NULL)
@@ -173,13 +177,15 @@  riscv_subset_list::to_string (bool version_p) const
       /* For !version_p, we only separate extension with underline for
 	 multi-letter extension.  */
       if (!first &&
-	  (version_p || subset->name.length() > 1))
+	  (version_p
+	   || subset->explicit_version_p
+	   || subset->name.length() > 1))
 	oss << '_';
       first = false;
 
       oss << subset->name;
 
-      if (version_p)
+      if (version_p || subset->explicit_version_p)
 	oss  << subset->major_version
 	     << 'p'
 	     << subset->minor_version;
@@ -240,7 +246,8 @@  riscv_supported_std_ext (void)
      `major_version` using default_major_version.
      `default_major_version`: Default major version.
      `default_minor_version`: Default minor version.
-     `std_ext_p`: True if parsing std extension.  */
+     `std_ext_p`: True if parsing std extension.
+     `explicit_version_p`: True if this subset is not using default version.  */
 
 const char *
 riscv_subset_list::parsing_subset_version (const char *p,
@@ -248,13 +255,15 @@  riscv_subset_list::parsing_subset_version (const char *p,
 					   unsigned *minor_version,
 					   unsigned default_major_version,
 					   unsigned default_minor_version,
-					   bool std_ext_p)
+					   bool std_ext_p,
+					   bool *explicit_version_p)
 {
   bool major_p = true;
   unsigned version = 0;
   unsigned major = 0;
   unsigned minor = 0;
   char np;
+  *explicit_version_p = false;
 
   for (; *p; ++p)
     {
@@ -269,6 +278,7 @@  riscv_subset_list::parsing_subset_version (const char *p,
 		{
 		  *major_version = version;
 		  *minor_version = 0;
+		  *explicit_version_p = true;
 		  return p;
 		}
 	      else
@@ -302,6 +312,7 @@  riscv_subset_list::parsing_subset_version (const char *p,
     }
   else
     {
+      *explicit_version_p = true;
       *major_version = major;
       *minor_version = minor;
     }
@@ -325,6 +336,7 @@  riscv_subset_list::parse_std_ext (const char *p)
   unsigned major_version = 0;
   unsigned minor_version = 0;
   char std_ext = '\0';
+  bool explicit_version_p = false;
 
   /* First letter must start with i, e or g.  */
   switch (*p)
@@ -334,8 +346,9 @@  riscv_subset_list::parse_std_ext (const char *p)
       p = parsing_subset_version (p, &major_version, &minor_version,
 				  /* default_major_version= */ 2,
 				  /* default_minor_version= */ 0,
-				  /* std_ext_p= */ true);
-      add ("i", major_version, minor_version);
+				  /* std_ext_p= */ true,
+				  &explicit_version_p);
+      add ("i", major_version, minor_version, explicit_version_p);
       break;
 
     case 'e':
@@ -343,9 +356,10 @@  riscv_subset_list::parse_std_ext (const char *p)
       p = parsing_subset_version (p, &major_version, &minor_version,
 				  /* default_major_version= */ 1,
 				  /* default_minor_version= */ 9,
-				  /* std_ext_p= */ true);
+				  /* std_ext_p= */ true,
+				  &explicit_version_p);
 
-      add ("e", major_version, minor_version);
+      add ("e", major_version, minor_version, explicit_version_p);
 
       if (m_xlen > 32)
 	{
@@ -360,13 +374,14 @@  riscv_subset_list::parse_std_ext (const char *p)
       p = parsing_subset_version (p, &major_version, &minor_version,
 				  /* default_major_version= */ 2,
 				  /* default_minor_version= */ 0,
-				  /* std_ext_p= */ true);
-      add ("i", major_version, minor_version);
+				  /* std_ext_p= */ true,
+				  &explicit_version_p);
+      add ("i", major_version, minor_version, explicit_version_p);
 
       for (; *std_exts != 'q'; std_exts++)
 	{
 	  const char subset[] = {*std_exts, '\0'};
-	  add (subset, major_version, minor_version);
+	  add (subset, major_version, minor_version, explicit_version_p);
 	}
       break;
 
@@ -413,24 +428,28 @@  riscv_subset_list::parse_std_ext (const char *p)
       p = parsing_subset_version (p, &major_version, &minor_version,
 				  /* default_major_version= */ 2,
 				  /* default_minor_version= */ 0,
-				  /* std_ext_p= */ true);
+				  /* std_ext_p= */ true,
+				  &explicit_version_p);
 
       subset[0] = std_ext;
 
-      handle_implied_ext (subset, major_version, minor_version);
+      handle_implied_ext (subset, major_version,
+			  minor_version, explicit_version_p);
 
-      add (subset, major_version, minor_version);
+      add (subset, major_version, minor_version, explicit_version_p);
     }
   return p;
 }
 
 
 /* Check any implied extensions for EXT with version
-   MAJOR_VERSION.MINOR_VERSION.  */
+   MAJOR_VERSION.MINOR_VERSION, EXPLICIT_VERSION_P indicate the version is
+   explicitly given by user or not.  */
 void
 riscv_subset_list::handle_implied_ext (const char *ext,
 				       int major_version,
-				       int minor_version)
+				       int minor_version,
+				       bool explicit_version_p)
 {
   riscv_implied_info_t *implied_info;
   for (implied_info = &riscv_implied_info[0];
@@ -445,7 +464,8 @@  riscv_subset_list::handle_implied_ext (const char *ext,
 	continue;
 
       /* TODO: Implied extension might use different version.  */
-      add (implied_info->implied_ext, major_version, minor_version);
+      add (implied_info->implied_ext, major_version, minor_version,
+	   explicit_version_p);
     }
 }
 
@@ -482,6 +502,7 @@  riscv_subset_list::parse_multiletter_ext (const char *p,
       char *subset = xstrdup (p);
       char *q = subset;
       const char *end_of_version;
+      bool explicit_version_p = false;
 
       while (*++q != '\0' && *q != '_' && !ISDIGIT (*q))
 	;
@@ -490,11 +511,12 @@  riscv_subset_list::parse_multiletter_ext (const char *p,
 	= parsing_subset_version (q, &major_version, &minor_version,
 				  /* default_major_version= */ 2,
 				  /* default_minor_version= */ 0,
-				  /* std_ext_p= */ FALSE);
+				  /* std_ext_p= */ FALSE,
+				  &explicit_version_p);
 
       *q = '\0';
 
-      add (subset, major_version, minor_version);
+      add (subset, major_version, minor_version, explicit_version_p);
       free (subset);
       p += end_of_version - subset;
 
diff --git a/gcc/testsuite/gcc.target/riscv/attribute-13.c b/gcc/testsuite/gcc.target/riscv/attribute-13.c
new file mode 100644
index 000000000000..1e8600132935
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/attribute-13.c
@@ -0,0 +1,6 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O -mriscv-attribute -march=rv32if3d -mabi=ilp32" } */
+int foo()
+{
+}
+/* { dg-final { scan-assembler ".attribute arch, \"rv32i2p0_f3p0_d2p0\"" } } */