| Message ID | 20230712032730.40158-1-lehua.ding@rivai.ai |
|---|---|
| State | New |
| Headers | show |
| Series | RISC-V: Throw compilation error for unknown sub-extension or supervisor extension | expand |
LGTM
juzhe.zhong@rivai.ai
From: Lehua Ding
Date: 2023-07-12 11:27
To: gcc-patches
CC: juzhe.zhong; rdapp.gcc; kito.cheng; palmer; jeffreyalaw
Subject: [PATCH] RISC-V: Throw compilation error for unknown sub-extension or supervisor extension
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.
e.g.:
Run `riscv64-unknown-elf-gcc -march=rv64gcv_zvl128_s123 a.c` will throw these error:
riscv64-unknown-elf-gcc: error: '-march=rv64gcv_zvl128_s123': extension 'zvl' starts with `z` but is not a standard sub-extension
riscv64-unknown-elf-gcc: error: '-march=rv64gcv_zvl128_s123': extension 's123' start with `s` but not a standard supervisor extension
Best,
Lehua
gcc/ChangeLog:
* common/config/riscv/riscv-common.cc (standard_extensions_p): New func.
(riscv_subset_list::add): Add check.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/arch-3.c: Update -march.
* 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-9.c: Ditto.
* gcc.target/riscv/pr102957.c: Ditto.
* gcc.target/riscv/arch-22.cc: New test.
---
gcc/common/config/riscv/riscv-common.cc | 29 +++++++++++++++++++
gcc/testsuite/gcc.target/riscv/arch-22.cc | 8 +++++
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-9.c | 4 +--
gcc/testsuite/gcc.target/riscv/pr102957.c | 2 ++
8 files changed, 45 insertions(+), 6 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..df3c256c80c 100644
--- a/gcc/common/config/riscv/riscv-common.cc
+++ b/gcc/common/config/riscv/riscv-common.cc
@@ -518,6 +518,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 +558,23 @@ riscv_subset_list::add (const char *subset, int major_version,
return;
}
+ else if (subset[0] == 'z' && !standard_extensions_p (subset))
+ {
+ error_at (m_loc,
+ "%<-march=%s%>: extension %qs starts with `z` but is not a "
+ "standard sub-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 not a standard "
+ "supervisor extension",
+ m_arch, subset);
+ return;
+ }
riscv_subset_t *s = new riscv_subset_t ();
riscv_subset_t *itr;
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..f9d8b57cb20
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/arch-22.cc
@@ -0,0 +1,8 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gcv_zvl128_z123_s123 -mabi=lp64d" } */
+int foo()
+{
+}
+/* { dg-error "extension 'zvl128' start with `z` but not a standard sub-extension" "" { target *-*-* } 0 } */
+/* { dg-error "extension 'z123' start with `z` but not a standard sub-extension" "" { target *-*-* } 0 } */
+/* { dg-error "extension 's123' start with `s` but not a standard supervisor 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..dee0fc6656d 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_xbar -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..8bdaa9d17b2 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_xbar -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-9.c b/gcc/testsuite/gcc.target/riscv/attribute-9.c
index 7e3741a827c..ec5bab963ae 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_xbar -mabi=ilp32e" } */
int foo()
{
}
-/* { dg-final { scan-assembler ".attribute arch, \"rv32i2p0_sabc_xbar\"" } } */
+/* { dg-final { scan-assembler ".attribute arch, \"rv32i2p0_svinval1p0_xbar\"" } } */
diff --git a/gcc/testsuite/gcc.target/riscv/pr102957.c b/gcc/testsuite/gcc.target/riscv/pr102957.c
index 9747dde3038..e11236b8504 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 not a standard sub-extension" "" { target *-*-* } 0 } */
On 7/11/23 21:30, juzhe.zhong@rivai.ai wrote:
> LGTM
OK for the trunk.
jeff
On Wed, 12 Jul 2023 09:02:06 PDT (-0700), jeffreyalaw@gmail.com wrote: > > > On 7/11/23 21:30, juzhe.zhong@rivai.ai wrote: >> LGTM > OK for the trunk. I'd like to make sure Kito is OK with this. IIUC the "pass through unknown extensions" behavior is deliberate. It's not what I would have done, but I didn't do it ;) > jeff
On 7/12/23 10:32, Palmer Dabbelt wrote: > On Wed, 12 Jul 2023 09:02:06 PDT (-0700), jeffreyalaw@gmail.com wrote: >> >> >> On 7/11/23 21:30, juzhe.zhong@rivai.ai wrote: >>> LGTM >> OK for the trunk. > > I'd like to make sure Kito is OK with this. IIUC the "pass through > unknown extensions" behavior is deliberate. It's not what I would have > done, but I didn't do it ;) OK. Not sure what the rationale behind that would be, but Kito may have had a good reason. Let's wait for his input. jeff
That's intentional before, since some time binutils may have supported that but the compiler doesn't, so GCC just bypasses that to binutils to let binutils reject those unknown extensions. But I am considering rejecting those extensions or adding more checks on the GCC side recently too, because accepting unknown extensions might cause problems on the architecture extension test macro[1], it makes the value become unreliable if the extension version info isn't in GCC yet. So I am OK with this change but two minor comments : --- > riscv64-unknown-elf-gcc: error: '-march=rv64gcv_zvl128_s123': extension 'zvl' starts with `z` but is not a standard sub-extension I would like to say it's `unsupported standard extension` rather than `not a standard sub-extension`. Because some extensions have just become ratified but GCC is unsupported yet, so `not a standard sub-extension` might confuse IMO. and why `extension` rather than `sub-extension`: IIRC `sub-extension` was used as an official term long ago, but it is called standard extension now[2]. ---- > riscv64-unknown-elf-gcc: error: '-march=rv64gcv_zvl128_s123': extension 's123' start with `s` but not a standard supervisor extension `is` is missing. ---- Also I would like to reject unknown single letter extensions and `x` extensions too, for the same reason as the other two, except that make the architecture extension test macro less useful. [1] https://github.com/riscv-non-isa/riscv-c-api-doc/blob/master/riscv-c-api.md#architecture-extension-test-macro [2] https://github.com/riscv/riscv-isa-manual/blob/main/src/naming.adoc#additional-standard-extension-names
Thanks for review. I uploaded version V2, which addresses Kito's comments,
along with two 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.
V2 URL: https://gcc.gnu.org/pipermail/gcc-patches/2023-July/624377.html
Best,
Lehua
------------------ Original ------------------
From: "Kito Cheng"<kito.cheng@gmail.com>;
Date: Thu, Jul 13, 2023 10:04 AM
To: "Jeff Law"<jeffreyalaw@gmail.com>;
Cc: "palmer"<palmer@rivosinc.com>; "juzhe.zhong"<juzhe.zhong@rivai.ai>; "lehua.ding"<lehua.ding@rivai.ai>; "gcc-patches"<gcc-patches@gcc.gnu.org>; "Robin Dapp"<rdapp.gcc@gmail.com>;
Subject: Re: [PATCH] RISC-V: Throw compilation error for unknown sub-extension or supervisor extension
That's intentional before, since some time binutils may have supported
that but the compiler doesn't, so GCC just bypasses that to binutils
to let binutils reject those unknown extensions.
But I am considering rejecting those extensions or adding more checks
on the GCC side recently too, because accepting unknown extensions
might cause problems on the architecture extension test macro[1], it
makes the value become unreliable if the extension version info isn't
in GCC yet.
So I am OK with this change but two minor comments :
---
> riscv64-unknown-elf-gcc: error: '-march=rv64gcv_zvl128_s123': extension 'zvl' starts with `z` but is not a standard sub-extension
I would like to say it's `unsupported standard extension` rather than
`not a standard sub-extension`.
Because some extensions have just become ratified but GCC is
unsupported yet, so `not a standard sub-extension` might confuse IMO.
and why `extension` rather than `sub-extension`: IIRC `sub-extension`
was used as an official term long ago, but it is called standard
extension now[2].
----
> riscv64-unknown-elf-gcc: error: '-march=rv64gcv_zvl128_s123': extension 's123' start with `s` but not a standard supervisor extension
`is` is missing.
----
Also I would like to reject unknown single letter extensions and `x`
extensions too, for the same reason as the other two, except that make
the architecture extension test macro less useful.
[1] https://github.com/riscv-non-isa/riscv-c-api-doc/blob/master/riscv-c-api.md#architecture-extension-test-macro
[2] https://github.com/riscv/riscv-isa-manual/blob/main/src/naming.adoc#additional-standard-extension-names
diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc index 6091d8f281b..df3c256c80c 100644 --- a/gcc/common/config/riscv/riscv-common.cc +++ b/gcc/common/config/riscv/riscv-common.cc @@ -518,6 +518,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 +558,23 @@ riscv_subset_list::add (const char *subset, int major_version, return; } + else if (subset[0] == 'z' && !standard_extensions_p (subset)) + { + error_at (m_loc, + "%<-march=%s%>: extension %qs starts with `z` but is not a " + "standard sub-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 not a standard " + "supervisor extension", + m_arch, subset); + return; + } riscv_subset_t *s = new riscv_subset_t (); riscv_subset_t *itr; 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..f9d8b57cb20 --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/arch-22.cc @@ -0,0 +1,8 @@ +/* { dg-do compile } */ +/* { dg-options "-march=rv64gcv_zvl128_z123_s123 -mabi=lp64d" } */ +int foo() +{ +} +/* { dg-error "extension 'zvl128' start with `z` but not a standard sub-extension" "" { target *-*-* } 0 } */ +/* { dg-error "extension 'z123' start with `z` but not a standard sub-extension" "" { target *-*-* } 0 } */ +/* { dg-error "extension 's123' start with `s` but not a standard supervisor 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..dee0fc6656d 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_xbar -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..8bdaa9d17b2 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_xbar -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-9.c b/gcc/testsuite/gcc.target/riscv/attribute-9.c index 7e3741a827c..ec5bab963ae 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_xbar -mabi=ilp32e" } */ int foo() { } -/* { dg-final { scan-assembler ".attribute arch, \"rv32i2p0_sabc_xbar\"" } } */ +/* { dg-final { scan-assembler ".attribute arch, \"rv32i2p0_svinval1p0_xbar\"" } } */ diff --git a/gcc/testsuite/gcc.target/riscv/pr102957.c b/gcc/testsuite/gcc.target/riscv/pr102957.c index 9747dde3038..e11236b8504 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 not a standard sub-extension" "" { target *-*-* } 0 } */