This looks pretty good to me, and looks like it works the same way as the binutils patches that already went in before the binutils-2.32 release. I just see some minor issues that need fixing.
On Tue, Feb 12, 2019 at 11:17 PM Kito Cheng <k...@andestech.com> wrote: > > From: Kito Cheng <kito.ch...@gmail.com> > > Kito Cheng <kito.ch...@gmail.com> > Monk Chiang <sh.chian...@gmail.com> > > ChangeLog: > gcc: > * common/config/riscv/riscv-common.c: > Include config/riscv/riscv-protos.h. > (INCLUDE_STRING): Defined. > (RISCV_DONT_CARE_VERSION): Defined. > (riscv_subset_t): Declare. Doesn't mention riscv_subset_t::riscv_subset_t. > (riscv_subset_list): Declare. > (riscv_subset_list::riscv_subset_list): New. > (riscv_subset_list::~riscv_subset_list): Likewise. > (riscv_subset_list::parsing_subset_version): Likewise. > (riscv_subset_list::parse_std_ext): Likewise. > (riscv_subset_list::parse_sv_or_non_std_ext): Likewise. > (riscv_subset_list::add): Likewise. > (riscv_subset_list::lookup): Likewise. > (riscv_subset_list::xlen): Likewise. > (riscv_subset_list::parse): Likewise. > (riscv_supported_std_ext): Likewise. > (current_subset_list): Likewise. > (riscv_parse_arch_string): Using riscv_subset_list::parse to > parse. > > gcc/testsuite: > * gcc.target/riscv/arch-1.c: New. > * gcc.target/riscv/arch-2.c: Likewise. > * gcc.target/riscv/arch-3.c: Likewise. > * gcc.target/riscv/arch-4.c: Likewise. > --- > gcc/common/config/riscv/riscv-common.c | 530 > ++++++++++++++++++++++++++++---- > gcc/testsuite/gcc.target/riscv/arch-1.c | 7 + > gcc/testsuite/gcc.target/riscv/arch-2.c | 6 + > gcc/testsuite/gcc.target/riscv/arch-3.c | 6 + > gcc/testsuite/gcc.target/riscv/arch-4.c | 6 + > 5 files changed, 496 insertions(+), 59 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/riscv/arch-1.c > create mode 100644 gcc/testsuite/gcc.target/riscv/arch-2.c > create mode 100644 gcc/testsuite/gcc.target/riscv/arch-3.c > create mode 100644 gcc/testsuite/gcc.target/riscv/arch-4.c > > diff --git a/gcc/common/config/riscv/riscv-common.c > b/gcc/common/config/riscv/riscv-common.c > index cb5bb7f..e412acb 100644 > --- a/gcc/common/config/riscv/riscv-common.c > +++ b/gcc/common/config/riscv/riscv-common.c > @@ -17,6 +17,7 @@ You should have received a copy of the GNU General Public > License > along with GCC; see the file COPYING3. If not see > <http://www.gnu.org/licenses/>. */ > > +#define INCLUDE_STRING > #include "config.h" > #include "system.h" > #include "coretypes.h" > @@ -26,99 +27,510 @@ along with GCC; see the file COPYING3. If not see > #include "opts.h" > #include "flags.h" > #include "diagnostic-core.h" > +#include "config/riscv/riscv-protos.h" > > -/* Parse a RISC-V ISA string into an option mask. Must clear or set all arch > - dependent mask bits, in case more than one -march string is passed. */ > +#define RISCV_DONT_CARE_VERSION -1 > > -static void > -riscv_parse_arch_string (const char *isa, int *flags, location_t loc) > +/* Subset info. */ > +struct riscv_subset_t { > + riscv_subset_t (); > + > + std::string name; > + int major_version; > + int minor_version; > + struct riscv_subset_t *next; > +}; > + > +/* Subset list. */ > +class riscv_subset_list { > +private: > + /* Original arch string. */ > + const char *m_arch; > + > + /* Localtion of arch string, used for report error. */ Localtion -> Location. > + location_t m_loc; > + > + /* Head of subset info list. */ > + riscv_subset_t *m_head; > + > + /* Tail of subset info list. */ > + riscv_subset_t *m_tail; > + > + /* X-len of m_arch. */ > + unsigned m_xlen; > + > + riscv_subset_list (const char *, location_t); > + > + const char *parsing_subset_version (const char *, unsigned *, unsigned *, > + unsigned, unsigned, bool); > + > + const char *parse_std_ext (const char *); > + > + const char *parse_sv_or_non_std_ext (const char *, const char *, > + const char *); > + > +public: > + ~riscv_subset_list (); > + > + void add (const char *, int, int); > + > + riscv_subset_t *lookup (const char *, > + int major_version = RISCV_DONT_CARE_VERSION, > + int minor_version = RISCV_DONT_CARE_VERSION) const; > + > + unsigned xlen() const {return m_xlen;}; > + > + static riscv_subset_list *parse (const char *, location_t); > + > +}; > + > +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) > { > - const char *p = isa; > +} > > - if (strncmp (p, "rv32", 4) == 0) > - *flags &= ~MASK_64BIT, p += 4; > - else if (strncmp (p, "rv64", 4) == 0) > - *flags |= MASK_64BIT, p += 4; > - else > +riscv_subset_list::riscv_subset_list (const char *arch, location_t loc) > +: m_arch (arch), m_loc(loc), m_head (NULL), m_tail (NULL), m_xlen(0) > +{ > +} > + > +riscv_subset_list::~riscv_subset_list() > +{ > + if (!m_head) > + return; > + > + riscv_subset_t *item = this->m_head->next; Looks like the first item in the list never gets freed. I think this should be just "this->m_head;". > + while (item != NULL) > { > - error_at (loc, "-march=%s: ISA string must begin with rv32 or rv64", > isa); > - return; > + riscv_subset_t *next = item->next; > + delete item; > + item = next; > } > +} > > - if (*p == 'g') > - { > - p++; > +/* Add new subset to list. */ > > - *flags &= ~MASK_RVE; > +void riscv_subset_list::add (const char *subset, > + int major_version, > + int minor_version) > +{ > + riscv_subset_t *s = new riscv_subset_t (); > > - *flags |= MASK_MUL; > - *flags |= MASK_ATOMIC; > - *flags |= MASK_HARD_FLOAT; > - *flags |= MASK_DOUBLE_FLOAT; > - } > - else if (*p == 'i') > - { > - p++; > + if (m_head == NULL) > + m_head = s; > + > + s->name = subset; > + s->major_version = major_version; > + s->minor_version = minor_version; > + s->next = NULL; > + > + if (m_tail != NULL) > + m_tail->next = s; > + > + m_tail = s; > +} > > - *flags &= ~MASK_RVE; > +/* Find subset in list without version checking, return NULL if not found. > */ Except this actually does do version checking, if versions are not RISCV_DONT_CARE_VERSION. The comment needs to be clarified. > > - *flags &= ~MASK_MUL; > - if (*p == 'm') > - *flags |= MASK_MUL, p++; > +riscv_subset_t *riscv_subset_list::lookup (const char *subset, > + int major_version, > + int minor_version) const > +{ > + riscv_subset_t *s; > + > + for (s = m_head; s != NULL; s = s->next) > + if (strcasecmp (s->name.c_str(), subset) == 0) > + { > + if ((major_version != RISCV_DONT_CARE_VERSION) > + && (s->major_version != major_version)) > + return NULL; > + > + if ((minor_version != RISCV_DONT_CARE_VERSION) > + && (s->minor_version != minor_version)) > + return NULL; > > - *flags &= ~MASK_ATOMIC; > - if (*p == 'a') > - *flags |= MASK_ATOMIC, p++; > + return s; > + } > + > + return s; > +} > + > +/* Return string which contain all supported standard extensions in contain -> contains > + canonical order. */ > + > +static const char * > +riscv_supported_std_ext (void) > +{ > + return "mafdqlcbjtpvn"; > +} > + > +/* Parsing subset version. > + > + Return Value: > + Points to the end of version > + > + Arguments: > + `p`: Curent parsing position. > + `major_version`: Parsing result of major version, using > + default_major_version if version is not present in arch string. > + `minor_version`: Parsing result of minor version, set to 0 if version is > + not present in arch string, but set to `default_minor_version` if > + `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. */ > + > +const char * > +riscv_subset_list::parsing_subset_version (const char *p, > + unsigned *major_version, > + unsigned *minor_version, > + unsigned default_major_version, > + unsigned default_minor_version, > + bool std_ext_p) > +{ > + bool major_p = true; > + unsigned version = 0; > + unsigned major = 0; > + unsigned minor = 0; > + char np; > > - *flags &= ~(MASK_HARD_FLOAT | MASK_DOUBLE_FLOAT); > - if (*p == 'f') > + for (;*p; ++p) > + { > + if (*p == 'p') > { > - *flags |= MASK_HARD_FLOAT, p++; > + np = *(p + 1); > > - if (*p == 'd') > + if (!ISDIGIT (np)) > { > - *flags |= MASK_DOUBLE_FLOAT; > - p++; > + /* Might be beginning of `p` extension. */ > + if (std_ext_p) > + { > + *major_version = version; > + *minor_version = 0; > + return p; > + } > + else > + { > + error_at (m_loc, "-march=%s: Expect number after `%dp'.", > + m_arch, version); > + return NULL; > + } > } > + > + major = version; > + major_p = false; > + version = 0; > } > + else if (ISDIGIT (*p)) > + version = (version * 10) + (*p - '0'); > + else > + break; > } > - else if (*p == 'e') > + > + if (major_p) > + major = version; > + else > + minor = version; > + > + if (major == 0 && minor == 0) > { > - p++; > + /* We don't found any version string, use default version. */ We don't found -> We didn't find > + *major_version = default_major_version; > + *minor_version = default_minor_version; > + } > + else > + { > + *major_version = major; > + *minor_version = minor; > + } > + return p; > +} > + > +/* Parsing function for standard extensions. > + > + Return Value: > + Points to the end of extensions. > + > + Arguments: > + `p`: Curent parsing position. */ > > - *flags |= MASK_RVE; > +const char * > +riscv_subset_list::parse_std_ext (const char *p) > +{ > + const char *all_std_exts = riscv_supported_std_ext (); > + const char *std_exts = all_std_exts; > + > + unsigned major_version = 0; > + unsigned minor_version = 0; > + char std_ext = '\0'; > + > + /* First letter must start with i, e or g. */ > + switch (*p) > + { > + case 'i': > + p++; > + p = parsing_subset_version ( > + p, &major_version, &minor_version, > + /* default_major_version= */ 2, > + /* default_minor_version= */ 0, > + /* std_ext_p= */true); This doesn't follow GNU style. There shouldn't be an open paren at the end of the line, just start putting the args there. > + add ("i", major_version, minor_version); > + break; > > - if (*flags & MASK_64BIT) > + case 'e': > + p++; > + p = parsing_subset_version ( > + p, &major_version, &minor_version, > + /* default_major_version= */ 1, > + /* default_minor_version= */ 9, > + /* std_ext_p= */true); This doesn't follow GNU style. There shouldn't be an open paren at the end of the line, just start putting the args there. > + > + add ("e", major_version, minor_version); > + > + if (m_xlen > 32) > + { > + error_at (m_loc, "-march=%s: rv%de is not a valid base ISA", > + m_arch, m_xlen); > + return NULL; > + } > + This blank line should be deleted. > + break; > + > + case 'g': > + p++; > + p = parsing_subset_version ( > + p, &major_version, &minor_version, > + /* default_major_version= */ 2, > + /* default_minor_version= */ 0, > + /* std_ext_p= */true); This doesn't follow GNU style. There shouldn't be an open paren at the end of the line, just start putting the args there. > + add ("i", major_version, minor_version); > + > + for ( ; *std_exts != 'q'; std_exts++) > + { > + const char subset[] = {*std_exts, '\0'}; > + add (subset, major_version, minor_version); > + } > + break; > + > + default: > + error_at (m_loc, "-march=%s: first ISA subset must be `e', `i' or > `g'", > + m_arch); > + return NULL; > + } > + > + while (*p) > + { > + char subset[2] = {0, 0}; > + > + if (*p == 'x' || *p == 's') > + break; > + > + if (*p == '_') > { > - error ("RV64E is not a valid base ISA"); > - return; > + p++; > + continue; > } > > - *flags &= ~MASK_MUL; > - if (*p == 'm') > - *flags |= MASK_MUL, p++; > + std_ext = *p; > + > + /* Checking canonical order. */ > + while (*std_exts && std_ext != *std_exts) std_exts++; > + > + if (std_ext != *std_exts) > + { > + if (strchr (all_std_exts, std_ext) == NULL) > + error_at (m_loc, "-march=%s: unsupported ISA subset `%c'", > + m_arch, *p); > + else > + error_at (m_loc, > + "-march=%s: ISA string is not in canonical order. `%c'", > + m_arch, *p); > + return NULL; > + } > > - *flags &= ~MASK_ATOMIC; > - if (*p == 'a') > - *flags |= MASK_ATOMIC, p++; > + std_exts++; > > - *flags &= ~(MASK_HARD_FLOAT | MASK_DOUBLE_FLOAT); > + p++; > + p = parsing_subset_version ( > + p, &major_version, &minor_version, > + /* default_major_version= */ 2, > + /* default_minor_version= */ 0, > + /* std_ext_p= */true); > + > + subset[0] = std_ext; > + > + add (subset, major_version, minor_version); > } > - else > + return p; > +} > + > +/* Parsing function for non-standard and supervisor extensions. > + > + Return Value: > + Points to the end of extensions. > + > + Arguments: > + `p`: Curent parsing position. > + `ext_type`: What kind of extensions, 'x', 's' or 'sx'. > + `ext_type_str`: Full name for kind of extension. */ > + > +const char * > +riscv_subset_list::parse_sv_or_non_std_ext (const char *p, > + const char *ext_type, > + const char *ext_type_str) > +{ > + unsigned major_version = 0; > + unsigned minor_version = 0; > + size_t ext_type_len = strlen (ext_type); > + > + while (*p) > { > - error_at (loc, "-march=%s: invalid ISA string", isa); > - return; > + if (*p == '_') > + { > + p++; > + continue; > + } > + > + if (strncmp (p, ext_type, ext_type_len) != 0) > + break; > + > + /* It's non-standard supervisor extension if it prefix with sx. */ > + if ((ext_type[0] == 's') && (ext_type_len == 1) > + && (*(p + 1) == 'x')) > + break; > + > + char *subset = xstrdup (p); > + char *q = subset; > + const char *end_of_version; > + > + while (*++q != '\0' && *q != '_' && !ISDIGIT (*q)) > + ; > + > + end_of_version = > + parsing_subset_version ( > + q, &major_version, &minor_version, > + /* default_major_version= */ 2, > + /* default_minor_version= */ 0, > + /* std_ext_p= */FALSE); This doesn't follow GNU style. There shouldn't be an open paren at the end of the line, just start putting the args there. > + > + *q = '\0'; > + > + add (subset, major_version, minor_version); > + free (subset); > + p += end_of_version - subset; > + > + if (*p != '\0' && *p != '_') > + { > + error_at (m_loc, "-march=%s: %s must seperate with _", seperate -> separate > + m_arch, ext_type_str); > + return NULL; > + } > } > > - *flags &= ~MASK_RVC; > - if (*p == 'c') > - *flags |= MASK_RVC, p++; > + return p; > +} > + > +/* Parsing arch string to subset list, return NULL if parsing failed. */ > > - if (*p) > +riscv_subset_list * > +riscv_subset_list::parse (const char *arch, location_t loc) > +{ > + riscv_subset_list *subset_list = new riscv_subset_list (arch, loc); > + const char *p = arch; > + if (strncmp (p, "rv32", 4) == 0) > { > - error_at (loc, "-march=%s: unsupported ISA substring %qs", isa, p); > - return; > + subset_list->m_xlen = 32; > + p += 4; > } > + else if (strncmp (p, "rv64", 4) == 0) > + { > + subset_list->m_xlen = 64; > + p += 4; > + } > + else > + { > + error_at (loc, "-march=%s: ISA string must begin with rv32 or rv64", > + arch); > + goto fail; > + } > + > + /* Parsing standard extension. */ > + p = subset_list->parse_std_ext (p); > + > + if (p == NULL) > + goto fail; > + > + /* Parsing non-standard extension. */ > + p = subset_list->parse_sv_or_non_std_ext ( > + p, "x", "non-standard extension"); > + > + if (p == NULL) > + goto fail; > + > + /* Parsing supervisor extension. */ > + p = subset_list->parse_sv_or_non_std_ext ( > + p, "s", "supervisor extension"); > + > + if (p == NULL) > + goto fail; > + > + /* Parsing non-standard supervisor extension. */ > + p = subset_list->parse_sv_or_non_std_ext ( > + p, "sx", "non-standard supervisor extension"); > + > + if (p == NULL) > + goto fail; > + > + return subset_list; > + > +fail: > + delete subset_list; > + return NULL; > +} > + > +/* Parse a RISC-V ISA string into an option mask. Must clear or set all arch > + dependent mask bits, in case more than one -march string is passed. */ > + > +static void > +riscv_parse_arch_string (const char *isa, int *flags, location_t loc) > +{ > + riscv_subset_list *subset_list; > + subset_list = riscv_subset_list::parse (isa, loc); > + if (!subset_list) > + return; > + > + if (subset_list->xlen () == 32) > + *flags &= ~MASK_64BIT; > + else if (subset_list->xlen () == 64) > + *flags |= MASK_64BIT; > + > + *flags &= ~MASK_RVE; > + if (subset_list->lookup ("e")) > + *flags |= MASK_RVE; > + > + *flags &= ~MASK_MUL; > + if (subset_list->lookup ("m")) > + *flags |= MASK_MUL; > + > + *flags &= ~MASK_ATOMIC; > + if (subset_list->lookup ("a")) > + *flags |= MASK_ATOMIC; > + > + *flags &= ~(MASK_HARD_FLOAT | MASK_DOUBLE_FLOAT); > + if (subset_list->lookup ("f")) > + *flags |= MASK_HARD_FLOAT; > + > + if (subset_list->lookup ("d")) > + *flags |= MASK_DOUBLE_FLOAT; > + > + if (current_subset_list) > + delete current_subset_list; > + > + current_subset_list = subset_list; > } > > /* Implement TARGET_HANDLE_OPTION. */ > diff --git a/gcc/testsuite/gcc.target/riscv/arch-1.c > b/gcc/testsuite/gcc.target/riscv/arch-1.c > new file mode 100644 > index 0000000..e7c8738 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/riscv/arch-1.c > @@ -0,0 +1,7 @@ > +/* Verify the return instruction is mret. */ This comment isn't valid for this testcase and should be dropped or updated. Likewise for all of the other testcases. > +/* { dg-do compile } */ > +/* { dg-options "-O -march=rv32I -mabi=ilp32" } */ > +int foo() > +{ > +} > +/* { dg-error ".-march=rv32I: first ISA subset must be `e', `i' or `g'" "" { > target *-*-* } 0 } */ The arch-1.c testcase fails for a 64-bit target. I get cc1: error: -march=rv32I: first ISA subset must be `e', `i' or `g'^M cc1: error: ABI requires -march=rv32^M compiler exited with status 1 PASS: gcc.target/riscv/arch-1.c (test for errors, line ) FAIL: gcc.target/riscv/arch-1.c (test for excess errors) the actual options passed to gcc by the testsuite infrastructure are -march=rv64gc -mabi=lp64d -march=rv32I -mabi=ilp32 since the rv32I fails, the rv64gc takes effect and conflicts with the ilp32 abi we can fix this by adding a -march=rv32i option after the failing one, we still get the error, and the last one wins so avoids the ilp32 abi conflict > diff --git a/gcc/testsuite/gcc.target/riscv/arch-2.c > b/gcc/testsuite/gcc.target/riscv/arch-2.c > new file mode 100644 > index 0000000..ed90e2f > --- /dev/null > +++ b/gcc/testsuite/gcc.target/riscv/arch-2.c > @@ -0,0 +1,6 @@ > +/* Verify the return instruction is mret. */ > +/* { dg-do compile } */ > +/* { dg-options "-O -march=rv32ixabc_xfoo -mabi=ilp32" } */ > +int foo() > +{ > +} > diff --git a/gcc/testsuite/gcc.target/riscv/arch-3.c > b/gcc/testsuite/gcc.target/riscv/arch-3.c > new file mode 100644 > index 0000000..55908dd > --- /dev/null > +++ b/gcc/testsuite/gcc.target/riscv/arch-3.c > @@ -0,0 +1,6 @@ > +/* Verify the return instruction is mret. */ > +/* { dg-do compile } */ > +/* { dg-options "-O -march=rv32ixbar_sabc_sxfoo -mabi=ilp32" } */ > +int foo() > +{ > +} > diff --git a/gcc/testsuite/gcc.target/riscv/arch-4.c > b/gcc/testsuite/gcc.target/riscv/arch-4.c > new file mode 100644 > index 0000000..3c9e1c1 > --- /dev/null > +++ b/gcc/testsuite/gcc.target/riscv/arch-4.c > @@ -0,0 +1,6 @@ > +/* Verify the return instruction is mret. */ > +/* { dg-do compile } */ > +/* { dg-options "-O -march=rv32i2p3_m4p2 -mabi=ilp32" } */ > +int foo() > +{ > +} > -- > 1.8.3.1 >
