[llvm] 6101d72 - [RISCV] Relax rules for ordering s/z/x prefixed extensions in ISA naming strings
Alex Bradbury via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 27 05:32:22 PDT 2023
Author: Alex Bradbury
Date: 2023-06-27T13:32:11+01:00
New Revision: 6101d720cb499f5ad19293475b429828fa3dbd75
URL: https://github.com/llvm/llvm-project/commit/6101d720cb499f5ad19293475b429828fa3dbd75
DIFF: https://github.com/llvm/llvm-project/commit/6101d720cb499f5ad19293475b429828fa3dbd75.diff
LOG: [RISCV] Relax rules for ordering s/z/x prefixed extensions in ISA naming strings
This was discussed somewhat in D148315. As it stands, we require in
RISCVISAInfo::parseArchString (used for e.g. -march parsing in Clang)
that extensions are given in the order of z, then s, then x prefixed
extensions (after the standard single-letter extensions). However, we
recently (in D148315) moved to that order from z/x/s as the canonical
ordering was changed in the spec. In addition, recent GCC seems to
require z* extensions before s*.
My recollection of the history here is that we thought keeping -march as
close to the rules for ISA naming strings as possible would simplify
things, as there's an existing spec to point to. My feeling is that now
we've had incompatible changes, and an incompatibility with GCC there's
no real benefit to sticking to this restriction, and it risks making it
much more painful than it needs to be to copy a -march= string between
GCC and Clang.
This patch removes all ordering restrictions so you can freely mix x/s/z
extensions.
To be very explicit, this doesn't change our behaviour when emitting a
canonically ordered extension string (e.g. in build attributes). We of
course sort according to the canonical order (as we understand it) in
that case.
Differential Revision: https://reviews.llvm.org/D149246
Added:
Modified:
clang/docs/ReleaseNotes.rst
clang/test/Driver/riscv-arch.c
llvm/docs/RISCVUsage.rst
llvm/lib/Support/RISCVISAInfo.cpp
llvm/unittests/Support/RISCVISAInfoTest.cpp
Removed:
################################################################################
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index bc6366b36fd4d..1ecf620f2a768 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -704,6 +704,9 @@ RISC-V Support
- Added ``attribute(riscv_rvv_vector_bits(__riscv_v_fixed_vlen))`` to allow
the size of a RVV (RISC-V Vector) scalable type to be specified. This allows
RVV scalable vector types to be used in structs or in global variables.
+- The rules for ordering of extensions in ``-march`` strings were relaxed. A
+ canonical ordering is no longer enforced on ``z*``, ``s*``, and ``x*``
+ prefixed extensions.
CUDA/HIP Language Changes
^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/test/Driver/riscv-arch.c b/clang/test/Driver/riscv-arch.c
index 8929d88e92c23..4f618d52dce10 100644
--- a/clang/test/Driver/riscv-arch.c
+++ b/clang/test/Driver/riscv-arch.c
@@ -323,8 +323,7 @@
// RUN: %clang --target=riscv32-unknown-elf -march=rv32ixdef_sabc -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-X-ORDER %s
// RV32-X-ORDER: error: invalid arch name 'rv32ixdef_sabc',
-// RV32-X-ORDER: standard supervisor-level extension not given
-// RV32-X-ORDER: in canonical order 'sabc'
+// RV32-X-ORDER unsupported non-standard user-level extension 'xdef'
// RUN: %clang --target=riscv32-unknown-elf -march=rv32ixabc_xabc -### %s \
// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-XDUP %s
diff --git a/llvm/docs/RISCVUsage.rst b/llvm/docs/RISCVUsage.rst
index 66c4aa61fe65b..f50ddf23d7d6a 100644
--- a/llvm/docs/RISCVUsage.rst
+++ b/llvm/docs/RISCVUsage.rst
@@ -40,6 +40,8 @@ The current known variances from the specification are:
users migrate build systems so as not to rely on this.
* Allowing CSRs to be named without gating on specific extensions. This
applies to all CSR names, not just those in zicsr, zicntr, and zihpm.
+* The ordering of ``z*``, ``s*``, and ``x*`` prefixed extension names is not
+ enforced in user-specified ISA naming strings (e.g. ``-march``).
We are actively deciding not to support multiple specification revisions
at this time. We acknowledge a likely future need, but actively defer the
diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index 58a98bc497a89..7b12abcdc801e 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -811,9 +811,9 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
// Parse the ISA string containing non-standard user-level
// extensions, standard supervisor-level extensions and
// non-standard supervisor-level extensions.
- // These extensions start with 'z', 's', 'x' prefixes, follow a
- // canonical order, might have a version number (major, minor)
- // and are separated by a single underscore '_'.
+ // These extensions start with 'z', 's', 'x' prefixes, might have a version
+ // number (major, minor) and are separated by a single underscore '_'. We do
+ // not enforce a canonical order for them.
// Set the hardware features for the extensions that are supported.
// Multi-letter extensions are seperated by a single underscore
@@ -822,9 +822,6 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
OtherExts.split(Split, '_');
SmallVector<StringRef, 8> AllExts;
- std::array<StringRef, 4> Prefix{"z", "s", "x"};
- auto I = Prefix.begin();
- auto E = Prefix.end();
if (Split.size() > 1 || Split[0] != "") {
for (StringRef Ext : Split) {
if (Ext.empty())
@@ -844,18 +841,6 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
"invalid extension prefix '" + Ext + "'");
}
- // Check ISA extensions are specified in the canonical order.
- while (I != E && *I != Type)
- ++I;
-
- if (I == E) {
- if (IgnoreUnknown)
- continue;
- return createStringError(errc::invalid_argument,
- "%s not given in canonical order '%s'",
- Desc.str().c_str(), Ext.str().c_str());
- }
-
if (!IgnoreUnknown && Name.size() == Type.size()) {
return createStringError(errc::invalid_argument,
"%s name missing after '%s'",
diff --git a/llvm/unittests/Support/RISCVISAInfoTest.cpp b/llvm/unittests/Support/RISCVISAInfoTest.cpp
index 71e0d752eaa75..cab91c43f1ceb 100644
--- a/llvm/unittests/Support/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/Support/RISCVISAInfoTest.cpp
@@ -192,7 +192,7 @@ TEST(ParseArchString, AcceptsSupportedBaseISAsAndSetsXLenAndFLen) {
EXPECT_EQ(InfoRV64G.getFLen(), 64U);
}
-TEST(ParseArchString, RequiresCanonicalOrderForExtensions) {
+TEST(ParseArchString, RequiresCanonicalOrderForSingleLetterExtensions) {
EXPECT_EQ(
toString(RISCVISAInfo::parseArchString("rv64idf", true).takeError()),
"standard user-level extension not given in canonical order 'f'");
@@ -203,12 +203,10 @@ TEST(ParseArchString, RequiresCanonicalOrderForExtensions) {
toString(
RISCVISAInfo::parseArchString("rv32i_zfinx_a", true).takeError()),
"invalid extension prefix 'a'");
- EXPECT_EQ(
- toString(RISCVISAInfo::parseArchString("rv64i_svnapot_zicsr", true)
- .takeError()),
- "standard user-level extension not given in canonical order 'zicsr'");
+ // Canonical ordering not required for z*, s*, and x* extensions.
EXPECT_THAT_EXPECTED(
- RISCVISAInfo::parseArchString("rv64imafdc_zicsr_svnapot", true),
+ RISCVISAInfo::parseArchString(
+ "rv64imafdc_xsfvcp_zicsr_xtheadba_svnapot_zawrs", true),
Succeeded());
}
More information about the llvm-commits
mailing list