[PATCH] D149246: [RISCV] Relax rules for ordering s/z/x prefixed extensions in ISA naming strings

[Alex Bradbury via Phabricator via cfe-commits]

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
 ^^^^^^^^^^^^^^^^^^^^^^^^^


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D149246.517103.patch
Type: text/x-patch
Size: 2996 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20230426/2733cc34/attachment.bin>