[llvm] c8318b9 - [RISCV] Fix canonical ordering of s* vs z* extensions in RISCVISAInfo

[Alex Bradbury via llvm-commits]

Author: Alex Bradbury
Date: 2023-04-19T07:04:48+01:00
New Revision: c8318b973ad0414a068f4b5025a896d912554a11

URL: https://github.com/llvm/llvm-project/commit/c8318b973ad0414a068f4b5025a896d912554a11
DIFF: https://github.com/llvm/llvm-project/commit/c8318b973ad0414a068f4b5025a896d912554a11.diff

LOG: [RISCV] Fix canonical ordering of s* vs z* extensions in RISCVISAInfo

As noted in https://reviews.llvm.org/D148315, the ordering logic for
OrderedExtensionMap currently puts s* before z* extensions, but per the
ISA manual the correct order should be z* and then s* (with the
exception of zxm*, which are ordered after s*).

This patch fixes the ordering and adds a TODO for zxm*. The changes are
visible in the test case added in
a35e67fc5be654a7efdfa6125343b90f8960a487 which also demonstrates an
issue with the ordering of single letter extensions (which isn't
addressed in this patch).

This ordering matches the one used by GCC/binutils as well.

Differential Revision: https://reviews.llvm.org/D148615

Added: 
    

Modified: 
    llvm/lib/Support/RISCVISAInfo.cpp
    llvm/unittests/Support/RISCVISAInfoTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index f3f53db574472..d5523ef6893ce 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -286,16 +286,17 @@ bool RISCVISAInfo::hasExtension(StringRef Ext) const {
 // We rank extensions in the following order:
 // -Single letter extensions in canonical order.
 // -Unknown single letter extensions in alphabetical order.
-// -Multi-letter extensions starting with 's' in alphabetical order.
 // -Multi-letter extensions starting with 'z' sorted by canonical order of
 //  the second letter then sorted alphabetically.
+// -Multi-letter extensions starting with 's' in alphabetical order.
+// -(TODO) Multi-letter extensions starting with 'zxm' in alphabetical order.
 // -X extensions in alphabetical order.
 // These flags are used to indicate the category. The first 6 bits store the
 // single letter extension rank for single letter and multi-letter extensions
 // starting with 'z'.
 enum RankFlags {
-  RF_S_EXTENSION = 1 << 6,
-  RF_Z_EXTENSION = 1 << 7,
+  RF_Z_EXTENSION = 1 << 6,
+  RF_S_EXTENSION = 1 << 7,
   RF_X_EXTENSION = 1 << 8,
 };
 

diff  --git a/llvm/unittests/Support/RISCVISAInfoTest.cpp b/llvm/unittests/Support/RISCVISAInfoTest.cpp
index afdf308269154..44f1abd972544 100644
--- a/llvm/unittests/Support/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/Support/RISCVISAInfoTest.cpp
@@ -484,11 +484,8 @@ TEST(OrderedExtensionMap, ExtensionsAreCorrectlyOrdered) {
   for (const auto &Ext : Exts)
     ExtNames.push_back(Ext.first);
 
-  // FIXME: z* extensions should be ordered before s* extensions. The current
-  // ordering matches what is documented in RISCVISAInfo, but this doesn't
-  // match the ISA manual.
   // FIXME: 'l' and 'y' should be ordered after 'i', 'm', 'c'.
   EXPECT_THAT(ExtNames,
-              ElementsAre("i", "m", "l", "c", "y", "sbar", "sfoo", "zicsr",
-                          "zmfoo", "zfinx", "zzfoo", "xbar", "xfoo"));
+              ElementsAre("i", "m", "l", "c", "y", "zicsr", "zmfoo", "zfinx",
+                           "zzfoo", "sbar", "sfoo", "xbar", "xfoo"));
 }