[RISCV] Modify arch string parsing order according to latest riscv spec

[joshua-arch1]

Summary

According to latest risc-v spec, the canonical order in which extension names must appear in the name string specified in Table 29.1 is different from before. In the latest table, non-standard extensions must be listed after all standard extensions. To keep consistent, we now change the parsing order in parseArchString().

[kito-cheng]

LGTM, but I would like wait @asb ack here to confirm this change.

---

I hate those rule changes...:(

[jrtc27]

The damage is probably limited in scope from a build system perspective given it’s only noticeable if you’re writing software that needs S extensions, ie privileged ones? This is still completely crap from RVI though and continues to make them look incompetent by making more breaking changes. @philipp.tomsich @cmuellner How can we push back against RVI doing this? Is GCC/binutils going to follow suit? And I guess we also have an issue on the psABI side since the arch string there is meant to be canonical… so maybe toolchains should NAK this and force RVI to backtrack?

[jrtc27]

Hm, I’m struggling to find a recent change in riscv-isa-manual. Did we just get this wrong before rather than it being a breaking change in the spec like you say in your summary? Please give more information as I don’t want to accuse RVI of bad practice if your statement isn’t correct…

[kito-cheng]

https://github.com/riscv/riscv-isa-manual/commit/84a7edb256d290cb436227c35a8a4f814fa41898

I did some archaeology today, seems like it not only change the order but also removed sx prefix.

GCC and binutils will follow that, we didn't follow that just because we didn't know that change…

[philipp.tomsich]

We need to remember that all of this is currently a draft document.
Ratification currently only ratifies extensions, but not changes to the entire document.

Next time this comes up for public comment, we need to highlight incompatible changes appropriately.

[joshua-arch1]

In D148315#4268326, @philipp.tomsich wrote:

We need to remember that all of this is currently a draft document.
Ratification currently only ratifies extensions, but not changes to the entire document.

Next time this comes up for public comment, we need to highlight incompatible changes appropriately.

I can see the order change mentioned in https://github.com/riscv/riscv-isa-manual/commit/84a7edb256d290cb436227c35a8a4f814fa41898 was commited early in Dec, 2018. I'm wondering why this change has not been ratified yet within more than 4 years.

[asb]

LGTM. I suppose a sensible follow-up patch would be to remove Sx altogether. I'm not aware of the prefix being used at all.

This specific patch only affects how we ingest ISA naming strings. The logic used for OrderedExtensionMap is separate, though looking at that seems to have an issue - it sorts S* then Z* then X*. I think this is probably due to misreading the table in the ISA manual (which I did when first composing this response!) - it's only the "Zxm*" machine-level extensions that should be sorted after S extensions, all other Z extensions should be before S.

parseNormalizedArchString, now used by LLD and llvm-objdump etc isn't strict about order, which is overall an advantage in this case as LLVM tooling isn't broken by changing (or fixing) the order. (EDIT: when ingesting binaries at least - you'd see problems with a .s that sets the attributes)

[joshua-arch1]

In D148315#4276536, @asb wrote:

LGTM. I suppose a sensible follow-up patch would be to remove Sx altogether. I'm not aware of the prefix being used at all.

This specific patch only affects how we ingest ISA naming strings. The logic used for OrderedExtensionMap is separate, though looking at that seems to have an issue - it sorts S* then Z* then X*. I think this is probably due to misreading the table in the ISA manual (which I did when first composing this response!) - it's only the "Zxm*" machine-level extensions that should be sorted after S extensions, all other Z extensions should be before S.

parseNormalizedArchString, now used by LLD and llvm-objdump etc isn't strict about order, which is overall an advantage in this case as LLVM tooling isn't broken by changing (or fixing) the order. (EDIT: when ingesting binaries at least - you'd see problems with a .s that sets the attributes)

Do we need to modify the values in RankFlags() for OrderedExtensionMap?

[asb]

@joshua-arch1: I've posted D146815 to fix the canonical ordering and directly committed rGa35e67fc5be654a7efdfa6125343b90f8960a487 to add some test coverage.

[asb]

I'm starting to think we should just remove the ordering rules for z/s/x altogether when parsing arch strings I see that gcc 12.2.0 actually requires s and then z:

[asb@purge ~]$ riscv64-linux-gnu-gcc -march=rv64imafdc_svinval_zicbom t.c -c
[asb@purge ~]$ riscv64-linux-gnu-gcc -march=rv64imafdc_zicbom_svinval t.c -c
riscv64-linux-gnu-gcc: error: '-march=rv64imafdc_zicbom_svinval': unexpected ISA string at end: 'svinval'

So ISA naming strings aren't easily portable between clang and GCC right now, even if the same extension names are recognised.

[joshua-arch1]

In D148315#4279486, @asb wrote:

I'm starting to think we should just remove the ordering rules for z/s/x altogether when parsing arch strings I see that gcc 12.2.0 actually requires s and then z:

[asb@purge ~]$ riscv64-linux-gnu-gcc -march=rv64imafdc_svinval_zicbom t.c -c
[asb@purge ~]$ riscv64-linux-gnu-gcc -march=rv64imafdc_zicbom_svinval t.c -c
riscv64-linux-gnu-gcc: error: '-march=rv64imafdc_zicbom_svinval': unexpected ISA string at end: 'svinval'

So ISA naming strings aren't easily portable between clang and GCC right now, even if the same extension names are recognised.

One of my colleagues working on GCC posted a patch to modify this order in GCC yesterday.
https://github.com/gcc-mirror/gcc/commit/4204ed2dc74390ab3689d1d6a53001761338baf6#diff-d6f7db0db31bfb339b01bec450f1b905381eb4730cc5ab2b2794971e34647d64
Also, Binutils requires z and then s, which is inconsistent with GCC now.

clang/test/Driver/riscv-arch.c

 // RV32S-UNS: error: invalid arch name 'rv32isa',
 // RV32S-UNS: unsupported standard supervisor-level extension 'sa'
 
 // RUN: %clang --target=riscv32-unknown-elf -march=rv32isxabc -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32SX-UNS %s
 // RV32SX-UNS: error: invalid arch name 'rv32isxabc',
 // RV32SX-UNS: unsupported non-standard supervisor-level extension 'sxabc'
 
-// RUN: %clang --target=riscv32-unknown-elf -march=rv32ixabc_sp_sxlw -### %s \
+// RUN: %clang --target=riscv32-unknown-elf -march=rv32isp_xabc_sxlw -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32ALL %s
-// RV32ALL: error: invalid arch name 'rv32ixabc_sp_sxlw',
-// RV32ALL: unsupported non-standard user-level extension 'xabc'
+// RV32ALL: error: invalid arch name 'rv32isp_xabc_sxlw',
+// RV32ALL: unsupported standard supervisor-level extension 'sp'
 
 // RUN: %clang --target=riscv32-unknown-elf -march=rv32i20 -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-IVER %s
 // RV32-IVER: error: invalid arch name 'rv32i20', unsupported
 // RV32-IVER: version number 20 for extension 'i'
 
 // RUN: %clang --target=riscv32-unknown-elf -march=rv32imc5 -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-CVER %s

 // RV32-XSEP: error: invalid arch name 'rv32ixabc_',
 // RV32-XSEP: extension name missing after separator '_'
 
 // 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=rv32isabc_xdef -### %s \
+// 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 'rv32isabc_xdef',
-// RV32-X-ORDER: non-standard user-level extension not given
-// RV32-X-ORDER: in canonical order 'xdef'
+// 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'
 
 // RUN: %clang --target=riscv32-unknown-elf -march=rv32isxabc_sdef -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-S-ORDER %s
 // RV32-S-ORDER: error: invalid arch name 'rv32isxabc_sdef',
 // RV32-S-ORDER: standard supervisor-level extension not given
 // RV32-S-ORDER: in canonical order 'sdef'
 
 // 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
 // RV32-X-X-INVAL: error: invalid arch name 'rv32ixabc_xdef', unsupported
 // RV32-X-X-INVAL: non-standard user-level extension 'xabc'
 
-// RUN: %clang --target=riscv32-unknown-elf -march=rv32ixabc_sdef_sxghi -### %s \
+// RUN: %clang --target=riscv32-unknown-elf -march=rv32isdef_xabc_sxghi -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-X-S-SX-INVAL %s
-// RV32-X-S-SX-INVAL: error: invalid arch name 'rv32ixabc_sdef_sxghi',
-// RV32-X-S-SX-INVAL: unsupported non-standard user-level extension 'xabc'
+// RV32-X-S-SX-INVAL: error: invalid arch name 'rv32isdef_xabc_sxghi',
+// RV32-X-S-SX-INVAL: unsupported standard supervisor-level extension 'sdef'
 
 // RUN: %clang --target=riscv32-unknown-elf -march=rv32i -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-TARGET %s
 // RUN: %clang --target=riscv64-unknown-elf -march=rv32i -### %s \
 // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-TARGET %s
 // RV32-TARGET: "-triple" "riscv32-unknown-unknown-elf"
 
 // RUN: %clang --target=riscv32-unknown-elf -march=rv64i -### %s \

llvm/lib/Support/RISCVISAInfo.cpp

for (auto I = Exts.begin(), E = Exts.end(); I != E;) {
     GoToNextExt(I, ConsumeLength);
   }
 
   // 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', 'x', 's', 'sx' prefixes, follow a
+  // These extensions start with 'z', 's', 's', 'sx' prefixes, follow a
   // canonical order, might have a version number (major, minor)
   // and are separated by a single underscore '_'.
   // 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", "x", "s", "sx"};
+  std::array<StringRef, 4> Prefix{"z", "s", "x", "sx"};
   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 '_'");