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

[asb]

Summary

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

[asb]

Add missing doc comment update.

[reames]

I generally think this is a good idea. I'd ask you add this to RISCVUsage under "The current known variances from the specification are", and mention it at the next sync up call, but otherwise, I think we should proceed with this.

LGTM once the above are done.

[palmer-dabbelt]

IDK about the specifics here, but I think in general being more permissive on ISA string parsing is the way to go. The rules have changed so many times it's not really viable to follow them, so we're sort of just stuck with a bunch of historally-driven implementation-specific behavior.

I've got a TODO floating around somewhere to document what GCC accepts, but it's super clunky and I get tired every time I try and look...

[ConchuOD]

Perhaps redundant now that Palmer has commented, an opinion was solicited from me so here I am, pretty much copy-pasting from elsewhere:

IMO, alignment with gcc is helpful, permissiveness is better.
Given "canonical order" can, and has, change(d) in the past I think it's good to insulate against it happening again.
Conjuring up march is bad enough as things stand w/ availability with given toolchain versions, especially in situations where different toolchain components are mixed between various versions llvm and gnu stuff.
Having to do special handing of different versions of gcc/clang whenever we do actually need to add Sfoo or Xfoo sounds like another layer of misery I would love to avoid.
Although it looks like we'll probably have to do some Makefile dance around older gcc/clang versions but at least going forward it'd be the same.

[kito-cheng]

I am really happy to see this happen, binutils has relaxed for a while in more relaxed way[1] - only require must start with rv[32|64][e|i|g], personally I would like to relax the order at all like binutils did for GCC, but I don't want to disturb that before making sure clang will going same way - otherwise just making more incomparable.

The order is kind of not friendly for user, I don't believe how many people remember the right order between z* extension in short time - even after read the RISC-V ISA spec.

[1] https://sourceware.org/pipermail/binutils/2022-December/125267.html

[asb]

All feedback so far has been positive and this has two LGTMs, but I also recognise this patch has been left for a while. Heads up that I intend to commit this towards the end of the working day Monday UK time unless anyone has any objections / concerns.

clang/docs/ReleaseNotes.rst

 - Fixed incorrect ABI lowering of ``_Float16`` in the case of structs
   containing ``_Float16`` that are eligible for passing via GPR+FPR or
   FPR+FPR.
 - Removed support for ``__attribute__((interrupt("user")))``. User-level
   interrupts are not in version 1.12 of the privileged specification.
 - 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
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 CUDA Support
 ^^^^^^^^^^^^
 - Clang now supports CUDA SDK up to 12.1
 

clang/test/Driver/riscv-arch.c

 // RUN: %clang --target=riscv32-unknown-elf -march=rv32ixabc_a -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-PREFIX %s
 // RV32-PREFIX: error: invalid arch name 'rv32ixabc_a',
 // RV32-PREFIX: invalid extension prefix 'a'
 
 // 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
 // RV32-XDUP: error: invalid arch name 'rv32ixabc_xabc',
 // RV32-XDUP: duplicated non-standard user-level extension 'xabc'
 
 // RUN: %clang --target=riscv32-unknown-elf -march=rv32ixabc_xdef -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-X-X-INVAL %s

llvm/docs/RISCVUsage.rst

* Unconditionally allowing instructions from zifencei, zicsr, zicntr, and
   zihpm without gating them on the extensions being enabled.  Previous
   revisions of the specification included these instructions in the base
   ISA, and we preserve this behavior to avoid breaking existing code.  If
   a future revision of the specification reuses these opcodes for other
   extensions, we may need to reevaluate this choice, and thus recommend
   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
 decisions making around handling this until we have a concrete example of
 real hardware having shipped and an incompatible change to the
 specification made afterwards.
 
 Base ISAs

llvm/lib/Support/RISCVISAInfo.cpp

for (auto I = Exts.begin(), E = Exts.end(); I != E;) {
     GoToNextExt(I, ConsumeLength, Exts.end());
   }
 
   // Handle other types of extensions other than the standard
   // general purpose and standard user-level extensions.
   // 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
   // as described in RISC-V User-Level ISA V2.2.
   SmallVector<StringRef, 8> Split;
   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())
         return createStringError(errc::invalid_argument,
                                  "extension name missing after separator '_'");
 
       StringRef Type = getExtensionType(Ext);
       StringRef Desc = getExtensionTypeDesc(Ext);
       auto Pos = findLastNonVersionCharacter(Ext) + 1;
       StringRef Name(Ext.substr(0, Pos));
       StringRef Vers(Ext.substr(Pos));
 
       if (Type.empty()) {
         if (IgnoreUnknown)
           continue;
         return createStringError(errc::invalid_argument,
                                  "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'",
                                  Desc.str().c_str(), Type.str().c_str());
       }
 
       unsigned Major, Minor, ConsumeLength;
       if (auto E = getExtensionVersion(Name, Vers, Major, Minor, ConsumeLength,

llvm/unittests/Support/RISCVISAInfoTest.cpp

TEST(ParseArchString, AcceptsSupportedBaseISAsAndSetsXLenAndFLen) {
   EXPECT_TRUE(ExtsRV64G.at("f") == (RISCVExtensionInfo{2, 2}));
   EXPECT_TRUE(ExtsRV64G.at("d") == (RISCVExtensionInfo{2, 2}));
   EXPECT_TRUE(ExtsRV64G.at("zicsr") == (RISCVExtensionInfo{2, 0}));
   EXPECT_TRUE(ExtsRV64G.at("zifencei") == (RISCVExtensionInfo{2, 0}));
   EXPECT_EQ(InfoRV64G.getXLen(), 64U);
   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'");
   EXPECT_EQ(
       toString(RISCVISAInfo::parseArchString("rv32iam", true).takeError()),
       "standard user-level extension not given in canonical order 'm'");
   EXPECT_EQ(
       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());
 }
 
 TEST(ParseArchString, RejectsUnrecognizedExtensionNamesByDefault) {
   EXPECT_EQ(toString(RISCVISAInfo::parseArchString("rv64ib", true).takeError()),
             "unsupported standard user-level extension 'b'");
   EXPECT_EQ(
       toString(