Re: [PATCH V2] RISC-V: Throw compilation error for unknown sub-extension or supervisor extension

[Kito Cheng <kito.cheng@gmail.com>]

LGTM, thanks :)

On Thu, Jul 13, 2023 at 4:33 PM Lehua Ding <lehua.ding@rivai.ai> wrote:
>
> Hi,
>
> This tiny patch add a check for extension starts with 'z' or 's' in `-march`
> option. Currently this unknown extension will be passed to the assembler, which
> then reports an error. With this patch, the compiler will throw a compilation
> error if the extension starts with 'z' or 's' is not a standard sub-extension or
> supervisor extension. Along with two extra changes. The first is to reduce
> repeated errors, which are currently reported at least twice. The second is to
> report as many mistakes as possible.
>
> e.g.:
>
> Run `riscv64-unknown-elf-gcc -march=rv64gvcw_zvl128_s123_x123 -mabi=lp64d a.c`
> will throw these error:
>
> riscv64-unknown-elf-gcc: error: '-march=rv64gcv_zvl128_s123': ISA string is not in canonical order. 'c'
> riscv64-unknown-elf-gcc: error: '-march=rv64gcv_zvl128_s123': extension 'w' is unsupported standard single letter extension
> riscv64-unknown-elf-gcc: error: '-march=rv64gcv_zvl128_s123': extension 'zvl128' start with `z` but is unsupported standard extension
> riscv64-unknown-elf-gcc: error: '-march=rv64gcv_zvl128_s123': extension 's123' start with `s` but is unsupported standard supervisor extension
> riscv64-unknown-elf-gcc: error: '-march=rv64gcv_zvl128_s123': extension 'x123' start with `x` but is unsupported non-standard extension
>
> Best,
> Lehua
>
> gcc/ChangeLog:
>
>         * common/config/riscv/riscv-common.cc (riscv_supported_std_ext): Init.
>         (standard_extensions_p): Add check.
>         (riscv_subset_list::add): Just return NULL if it failed before.
>         (riscv_subset_list::parse_std_ext): Continue parse when find a error
>         (riscv_subset_list::parse): Just return NULL if it failed before.
>         * config/riscv/riscv-subset.h (class riscv_subset_list): Add field.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/riscv/arch-2.c: Update -march.
>         * gcc.target/riscv/arch-3.c: Ditto.
>         * gcc.target/riscv/arch-5.c: Ditto.
>         * gcc.target/riscv/arch-8.c: Ditto.
>         * gcc.target/riscv/attribute-10.c: Ditto.
>         * gcc.target/riscv/attribute-18.c: Ditto.
>         * gcc.target/riscv/attribute-19.c: Ditto.
>         * gcc.target/riscv/attribute-8.c: Ditto.
>         * gcc.target/riscv/attribute-9.c: Ditto.
>         * gcc.target/riscv/pr102957.c: Ditto.
>         * gcc.target/riscv/arch-22.cc: New test.
>
> ---
>  gcc/common/config/riscv/riscv-common.cc       | 68 +++++++++++++++----
>  gcc/config/riscv/riscv-subset.h               |  5 ++
>  gcc/testsuite/gcc.target/riscv/arch-2.c       |  2 +-
>  gcc/testsuite/gcc.target/riscv/arch-22.cc     | 11 +++
>  gcc/testsuite/gcc.target/riscv/arch-3.c       |  2 +-
>  gcc/testsuite/gcc.target/riscv/arch-5.c       |  2 +-
>  gcc/testsuite/gcc.target/riscv/arch-8.c       |  2 +-
>  gcc/testsuite/gcc.target/riscv/attribute-10.c |  2 +-
>  gcc/testsuite/gcc.target/riscv/attribute-18.c |  4 +-
>  gcc/testsuite/gcc.target/riscv/attribute-19.c |  4 +-
>  gcc/testsuite/gcc.target/riscv/attribute-8.c  |  4 +-
>  gcc/testsuite/gcc.target/riscv/attribute-9.c  |  4 +-
>  gcc/testsuite/gcc.target/riscv/pr102957.c     |  2 +
>  13 files changed, 87 insertions(+), 25 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/arch-22.cc
>
> diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
> index 6091d8f281b..9de7c54269e 100644
> --- a/gcc/common/config/riscv/riscv-common.cc
> +++ b/gcc/common/config/riscv/riscv-common.cc
> @@ -311,6 +311,8 @@ static const char *riscv_tunes[] =
>
>  static const char *riscv_supported_std_ext (void);
>
> +bool riscv_subset_list::parse_failed = false;
> +
>  static riscv_subset_list *current_subset_list = NULL;
>
>  const riscv_subset_list *riscv_current_subset_list ()
> @@ -518,6 +520,18 @@ subset_cmp (const std::string &a, const std::string &b)
>      }
>  }
>
> +/* Return true if EXT is a standard extension.  */
> +
> +static bool
> +standard_extensions_p (const char *ext)
> +{
> +  const riscv_ext_version *ext_ver;
> +  for (ext_ver = &riscv_ext_version_table[0]; ext_ver->name != NULL; ++ext_ver)
> +    if (strcmp (ext, ext_ver->name) == 0)
> +      return true;
> +  return false;
> +}
> +
>  /* Add new subset to list.  */
>
>  void
> @@ -546,6 +560,38 @@ riscv_subset_list::add (const char *subset, int major_version,
>
>        return;
>      }
> +  else if (strlen (subset) == 1 && !standard_extensions_p (subset))
> +    {
> +      error_at (m_loc,
> +               "%<-march=%s%>: extension %qs is unsupported standard single "
> +               "letter extension",
> +               m_arch, subset);
> +      return;
> +    }
> +  else if (subset[0] == 'z' && !standard_extensions_p (subset))
> +    {
> +      error_at (m_loc,
> +               "%<-march=%s%>: extension %qs starts with `z` but is "
> +               "unsupported standard extension",
> +               m_arch, subset);
> +      return;
> +    }
> +  else if (subset[0] == 's' && !standard_extensions_p (subset))
> +    {
> +      error_at (m_loc,
> +               "%<-march=%s%>: extension %qs start with `s` but is "
> +               "unsupported standard supervisor extension",
> +               m_arch, subset);
> +      return;
> +    }
> +  else if (subset[0] == 'x' && !standard_extensions_p (subset))
> +    {
> +      error_at (m_loc,
> +               "%<-march=%s%>: extension %qs start with `x` but is "
> +               "unsupported non-standard extension",
> +               m_arch, subset);
> +      return;
> +    }
>
>    riscv_subset_t *s = new riscv_subset_t ();
>    riscv_subset_t *itr;
> @@ -919,22 +965,16 @@ riscv_subset_list::parse_std_ext (const char *p)
>        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;
> -       }
> +      subset[0] = std_ext;
> +      if (std_ext != *std_exts && standard_extensions_p (subset))
> +       error_at (m_loc,
> +                 "%<-march=%s%>: ISA string is not in canonical order. "
> +                 "%<%c%>",
> +                 m_arch, *p);
>
>        std_exts++;
>
>        p++;
> -      subset[0] = std_ext;
>
>        p = parsing_subset_version (subset, p, &major_version, &minor_version,
>                                   /* std_ext_p= */ true, &explicit_version_p);
> @@ -1139,6 +1179,9 @@ riscv_subset_list::parse_multiletter_ext (const char *p,
>  riscv_subset_list *
>  riscv_subset_list::parse (const char *arch, location_t loc)
>  {
> +  if (riscv_subset_list::parse_failed)
> +    return NULL;
> +
>    riscv_subset_list *subset_list = new riscv_subset_list (arch, loc);
>    riscv_subset_t *itr;
>    const char *p = arch;
> @@ -1205,6 +1248,7 @@ riscv_subset_list::parse (const char *arch, location_t loc)
>
>  fail:
>    delete subset_list;
> +  riscv_subset_list::parse_failed = true;
>    return NULL;
>  }
>
> diff --git a/gcc/config/riscv/riscv-subset.h b/gcc/config/riscv/riscv-subset.h
> index 92e4fb31692..6691997df7c 100644
> --- a/gcc/config/riscv/riscv-subset.h
> +++ b/gcc/config/riscv/riscv-subset.h
> @@ -41,6 +41,11 @@ struct riscv_subset_t
>  /* Subset list.  */
>  class riscv_subset_list
>  {
> +public:
> +  /* Because the parse method is called in several places, to prevent repeated
> +     errors, use this flag to prevent it from repeating parse. */
> +  static bool parse_failed;
> +
>  private:
>    /* Original arch string.  */
>    const char *m_arch;
> diff --git a/gcc/testsuite/gcc.target/riscv/arch-2.c b/gcc/testsuite/gcc.target/riscv/arch-2.c
> index 8908b4b3a8c..e308bf7407c 100644
> --- a/gcc/testsuite/gcc.target/riscv/arch-2.c
> +++ b/gcc/testsuite/gcc.target/riscv/arch-2.c
> @@ -1,5 +1,5 @@
>  /* { dg-do compile } */
> -/* { dg-options "-march=rv32ixabc_xfoo -mabi=ilp32" } */
> +/* { dg-options "-march=rv32ixtheadba_xtheadsync -mabi=ilp32" } */
>  int foo()
>  {
>  }
> diff --git a/gcc/testsuite/gcc.target/riscv/arch-22.cc b/gcc/testsuite/gcc.target/riscv/arch-22.cc
> new file mode 100644
> index 00000000000..68337a70406
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/arch-22.cc
> @@ -0,0 +1,11 @@
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gvcw_zvl128_s123_x123 -mabi=lp64d" } */
> +int foo()
> +{
> +}
> +
> +/* { dg-error "ISA string is not in canonical order. 'c'" "" { target *-*-* } 0 } */
> +/* { dg-error "extension 'w' is unsupported standard single letter extension" "" { target *-*-* } 0 } */
> +/* { dg-error "extension 'zvl128' start with `z` but is unsupported standard extension" "" { target *-*-* } 0 } */
> +/* { dg-error "extension 's123' start with `s` but is unsupported standard supervisor extension" "" { target *-*-* } 0 } */
> +/* { dg-error "extension 'x123' start with `x` but is unsupported non-standard extension" "" { target *-*-* } 0 } */
> diff --git a/gcc/testsuite/gcc.target/riscv/arch-3.c b/gcc/testsuite/gcc.target/riscv/arch-3.c
> index 7aa945eca20..ac733746fd6 100644
> --- a/gcc/testsuite/gcc.target/riscv/arch-3.c
> +++ b/gcc/testsuite/gcc.target/riscv/arch-3.c
> @@ -1,5 +1,5 @@
>  /* { dg-do compile } */
> -/* { dg-options "-march=rv32isabc_xbar -mabi=ilp32" } */
> +/* { dg-options "-march=rv32isvinval_xtheadba -mabi=ilp32" } */
>  int foo()
>  {
>  }
> diff --git a/gcc/testsuite/gcc.target/riscv/arch-5.c b/gcc/testsuite/gcc.target/riscv/arch-5.c
> index 8258552214f..b8b6cd048f7 100644
> --- a/gcc/testsuite/gcc.target/riscv/arch-5.c
> +++ b/gcc/testsuite/gcc.target/riscv/arch-5.c
> @@ -1,5 +1,5 @@
>  /* { dg-do compile } */
> -/* { dg-options "-march=rv32i_zfoo_sabc_xbar -mabi=ilp32" } */
> +/* { dg-options "-march=rv32i_zmmul_svnapot_xtheadba -mabi=ilp32" } */
>  int foo()
>  {
>  }
> diff --git a/gcc/testsuite/gcc.target/riscv/arch-8.c b/gcc/testsuite/gcc.target/riscv/arch-8.c
> index 1b9e51b0e12..ef557aeb673 100644
> --- a/gcc/testsuite/gcc.target/riscv/arch-8.c
> +++ b/gcc/testsuite/gcc.target/riscv/arch-8.c
> @@ -1,5 +1,5 @@
>  /* { dg-do compile } */
> -/* { dg-options "-march=rv32id_zicsr_zifence -mabi=ilp32" } */
> +/* { dg-options "-march=rv32id_zicsr_zifencei -mabi=ilp32" } */
>  int foo()
>  {
>  }
> diff --git a/gcc/testsuite/gcc.target/riscv/attribute-10.c b/gcc/testsuite/gcc.target/riscv/attribute-10.c
> index 1e121a10753..868adef6ab7 100644
> --- a/gcc/testsuite/gcc.target/riscv/attribute-10.c
> +++ b/gcc/testsuite/gcc.target/riscv/attribute-10.c
> @@ -1,5 +1,5 @@
>  /* { dg-do compile } */
> -/* { dg-options "-march=rv32i -march=rv32im_sx_unexpectedstring -mabi=ilp32" } */
> +/* { dg-options "-march=rv32i -march=rv32im_svnapot_unexpectedstring -mabi=ilp32" } */
>  int foo()
>  {
>  }
> diff --git a/gcc/testsuite/gcc.target/riscv/attribute-18.c b/gcc/testsuite/gcc.target/riscv/attribute-18.c
> index 492360cf7c1..fddbf15fc3e 100644
> --- a/gcc/testsuite/gcc.target/riscv/attribute-18.c
> +++ b/gcc/testsuite/gcc.target/riscv/attribute-18.c
> @@ -1,4 +1,4 @@
>  /* { dg-do compile } */
> -/* { dg-options "-mriscv-attribute -march=rv64imafdcp -mabi=lp64d -misa-spec=2.2" } */
> +/* { dg-options "-mriscv-attribute -march=rv64imafdc -mabi=lp64d -misa-spec=2.2" } */
>  int foo() {}
> -/* { dg-final { scan-assembler ".attribute arch, \"rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0_p\"" } } */
> +/* { dg-final { scan-assembler ".attribute arch, \"rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0\"" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/attribute-19.c b/gcc/testsuite/gcc.target/riscv/attribute-19.c
> index 562f8089361..8150452f5b1 100644
> --- a/gcc/testsuite/gcc.target/riscv/attribute-19.c
> +++ b/gcc/testsuite/gcc.target/riscv/attribute-19.c
> @@ -1,4 +1,4 @@
>  /* { dg-do compile } */
> -/* { dg-options "-mriscv-attribute -march=rv64imp0p9 -mabi=lp64 -misa-spec=2.2" } */
> +/* { dg-options "-mriscv-attribute -march=rv64im -mabi=lp64 -misa-spec=2.2" } */
>  int foo() {}
> -/* { dg-final { scan-assembler ".attribute arch, \"rv64i2p0_m2p0_p0p9\"" } } */
> +/* { dg-final { scan-assembler ".attribute arch, \"rv64i2p0_m2p0\"" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/attribute-8.c b/gcc/testsuite/gcc.target/riscv/attribute-8.c
> index 4c91b9e6369..2065f74b977 100644
> --- a/gcc/testsuite/gcc.target/riscv/attribute-8.c
> +++ b/gcc/testsuite/gcc.target/riscv/attribute-8.c
> @@ -1,6 +1,6 @@
>  /* { dg-do compile } */
> -/* { dg-options "-mriscv-attribute -march=rv32i2p0xabc_xv5 -mabi=ilp32" } */
> +/* { dg-options "-mriscv-attribute -march=rv32i2p0xtheadba_xtheadbb -mabi=ilp32" } */
>  int foo()
>  {
>  }
> -/* { dg-final { scan-assembler ".attribute arch, \"rv32i2p0_xabc_xv5p0\"" } } */
> +/* { dg-final { scan-assembler ".attribute arch, \"rv32i2p0_xtheadba1p0_xtheadbb1p0\"" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/attribute-9.c b/gcc/testsuite/gcc.target/riscv/attribute-9.c
> index 7e3741a827c..3c31848ed80 100644
> --- a/gcc/testsuite/gcc.target/riscv/attribute-9.c
> +++ b/gcc/testsuite/gcc.target/riscv/attribute-9.c
> @@ -1,6 +1,6 @@
>  /* { dg-do compile } */
> -/* { dg-options "-mriscv-attribute -march=rv32i2p0sabc_xbar -mabi=ilp32e" } */
> +/* { dg-options "-mriscv-attribute -march=rv32i2p0svinval_xtheadba -mabi=ilp32e" } */
>  int foo()
>  {
>  }
> -/* { dg-final { scan-assembler ".attribute arch, \"rv32i2p0_sabc_xbar\"" } } */
> +/* { dg-final { scan-assembler ".attribute arch, \"rv32i2p0_svinval1p0_xtheadba1p0\"" } } */
> diff --git a/gcc/testsuite/gcc.target/riscv/pr102957.c b/gcc/testsuite/gcc.target/riscv/pr102957.c
> index 9747dde3038..322d49c62d4 100644
> --- a/gcc/testsuite/gcc.target/riscv/pr102957.c
> +++ b/gcc/testsuite/gcc.target/riscv/pr102957.c
> @@ -3,3 +3,5 @@
>  int foo()
>  {
>  }
> +
> +/* { dg-error "extension 'zb' starts with `z` but is unsupported standard extension" "" { target *-*-* } 0 } */
> --
> 2.36.1
>