asb created this revision.
asb added reviewers: reames, kito-cheng, craig.topper, jrtc27, joshua-arch1.
Herald added subscribers: jobnoorman, luke, wingo, pmatos, VincentWu, vkmr,
frasercrmck, evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim,
benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01,
edward-jones, zzheng, shiva0217, niosHD, sabuasal, simoncook, johnrusso, rbar,
hiraditya, arichardson.
Herald added a project: All.
asb requested review of this revision.
Herald added subscribers: cfe-commits, pcwang-thead, eopXD, MaskRay.
Herald added projects: clang, LLVM.
This was discussed somewhat in D148315 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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 actually removes all ordering restrictions so you can freely mix
x/s/z extensions. Arguably this is more freedom than we want to allow, on the
other hand it might be less hassle for build systems assembling their arch
strings.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D149246
Files:
clang/docs/ReleaseNotes.rst
llvm/lib/Support/RISCVISAInfo.cpp
llvm/unittests/Support/RISCVISAInfoTest.cpp
Index: llvm/unittests/Support/RISCVISAInfoTest.cpp
===================================================================
--- llvm/unittests/Support/RISCVISAInfoTest.cpp
+++ llvm/unittests/Support/RISCVISAInfoTest.cpp
@@ -192,7 +192,7 @@
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 @@
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());
}
Index: llvm/lib/Support/RISCVISAInfo.cpp
===================================================================
--- llvm/lib/Support/RISCVISAInfo.cpp
+++ llvm/lib/Support/RISCVISAInfo.cpp
@@ -767,9 +767,6 @@
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())
@@ -789,18 +786,6 @@
"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'",
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -422,6 +422,9 @@
- Fixed incorrect ABI lowering of ``_Float16`` in the case of structs
containing ``_Float16`` that are eligible for passing via GPR+FPR or
FPR+FPR.
+- 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
^^^^^^^^^^^^^^^^^^^^^^^^^
Index: llvm/unittests/Support/RISCVISAInfoTest.cpp
===================================================================
--- llvm/unittests/Support/RISCVISAInfoTest.cpp
+++ llvm/unittests/Support/RISCVISAInfoTest.cpp
@@ -192,7 +192,7 @@
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 @@
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());
}
Index: llvm/lib/Support/RISCVISAInfo.cpp
===================================================================
--- llvm/lib/Support/RISCVISAInfo.cpp
+++ llvm/lib/Support/RISCVISAInfo.cpp
@@ -767,9 +767,6 @@
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())
@@ -789,18 +786,6 @@
"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'",
Index: clang/docs/ReleaseNotes.rst
===================================================================
--- clang/docs/ReleaseNotes.rst
+++ clang/docs/ReleaseNotes.rst
@@ -422,6 +422,9 @@
- Fixed incorrect ABI lowering of ``_Float16`` in the case of structs
containing ``_Float16`` that are eligible for passing via GPR+FPR or
FPR+FPR.
+- 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
^^^^^^^^^^^^^^^^^^^^^^^^^
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
