[RISCV] Assert extensions are sorted at compile time. NFCI

[lukel97]

This replaces verifyTables and assert(llvm::is_sorted) with a constexpr
static_assert, so that adding an extension or implication in the wrong order
will be a compile time failure rather than a runtime failure.

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-llvm-support

Author: Luke Lau (lukel97)

Changes

This replaces verifyTables and assert(llvm::is_sorted) with a constexpr
static_assert, so that adding an extension or implication in the wrong order
will be a compile time failure rather than a runtime failure.

---

Full diff: https://github.com/llvm/llvm-project/pull/77442.diff

1 Files Affected:

• (modified) llvm/lib/Support/RISCVISAInfo.cpp (+80-82)

diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index 70f531e40b90e6..07828795352a3d 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -16,7 +16,6 @@
 #include "llvm/Support/raw_ostream.h"
 
 #include <array>
-#include <atomic>
 #include <optional>
 #include <string>
 #include <vector>
@@ -35,8 +34,8 @@ struct RISCVSupportedExtension {
   /// Supported version.
   RISCVExtensionVersion Version;
 
-  bool operator<(const RISCVSupportedExtension &RHS) const {
-    return StringRef(Name) < StringRef(RHS.Name);
+  constexpr bool operator<(const RISCVSupportedExtension &RHS) const {
+    return std::string_view(Name) < std::string_view(RHS.Name);
   }
 };
 
@@ -49,7 +48,7 @@ static const char *RISCVGImplications[] = {
 };
 
 // NOTE: This table should be sorted alphabetically by extension name.
-static const RISCVSupportedExtension SupportedExtensions[] = {
+static constexpr RISCVSupportedExtension SupportedExtensions[] = {
     {"a", RISCVExtensionVersion{2, 1}},
     {"c", RISCVExtensionVersion{2, 0}},
     {"d", RISCVExtensionVersion{2, 2}},
@@ -187,7 +186,7 @@ static const RISCVSupportedExtension SupportedExtensions[] = {
 };
 
 // NOTE: This table should be sorted alphabetically by extension name.
-static const RISCVSupportedExtension SupportedExperimentalExtensions[] = {
+static constexpr RISCVSupportedExtension SupportedExperimentalExtensions[] = {
     {"zacas", RISCVExtensionVersion{1, 0}},
 
     {"zcmop", RISCVExtensionVersion{0, 2}},
@@ -207,19 +206,19 @@ static const RISCVSupportedExtension SupportedExperimentalExtensions[] = {
     {"zvfbfwma", RISCVExtensionVersion{0, 8}},
 };
 
-static void verifyTables() {
-#ifndef NDEBUG
-  static std::atomic<bool> TableChecked(false);
-  if (!TableChecked.load(std::memory_order_relaxed)) {
-    assert(llvm::is_sorted(SupportedExtensions) &&
-           "Extensions are not sorted by name");
-    assert(llvm::is_sorted(SupportedExperimentalExtensions) &&
-           "Experimental extensions are not sorted by name");
-    TableChecked.store(true, std::memory_order_relaxed);
-  }
-#endif
+// TODO: Replace with std::is_sorted once we move to C++20
+template <typename T> constexpr bool isSorted(T &&Extensions) {
+  for (size_t I = 0; I < std::size(Extensions) - 1; I++)
+    if (!(Extensions[I] < Extensions[I + 1]))
+      return false;
+  return true;
 }
 
+static_assert(isSorted(SupportedExtensions) &&
+              "Extensions are not sorted by name");
+static_assert(isSorted(SupportedExperimentalExtensions) &&
+              "Experimental extensions are not sorted by name");
+
 static void PrintExtension(StringRef Name, StringRef Version,
                            StringRef Description) {
   outs().indent(4);
@@ -359,8 +358,6 @@ bool RISCVISAInfo::isSupportedExtensionFeature(StringRef Ext) {
 }
 
 bool RISCVISAInfo::isSupportedExtension(StringRef Ext) {
-  verifyTables();
-
   for (auto ExtInfo : {ArrayRef(SupportedExtensions),
                        ArrayRef(SupportedExperimentalExtensions)}) {
     auto I = llvm::lower_bound(ExtInfo, Ext, LessExtName());
@@ -1062,11 +1059,11 @@ static const char *ImpliedExtsZvl65536b[] = {"zvl32768b"};
 static const char *ImpliedExtsZvl8192b[] = {"zvl4096b"};
 
 struct ImpliedExtsEntry {
-  StringLiteral Name;
+  const char *Name;
   ArrayRef<const char *> Exts;
 
-  bool operator<(const ImpliedExtsEntry &Other) const {
-    return Name < Other.Name;
+  constexpr bool operator<(const ImpliedExtsEntry &Other) const {
+    return std::string_view(Name) < std::string_view(Other.Name);
   }
 
   bool operator<(StringRef Other) const { return Name < Other; }
@@ -1074,67 +1071,70 @@ struct ImpliedExtsEntry {
 
 // Note: The table needs to be sorted by name.
 static constexpr ImpliedExtsEntry ImpliedExts[] = {
-    {{"d"}, {ImpliedExtsD}},
-    {{"f"}, {ImpliedExtsF}},
-    {{"v"}, {ImpliedExtsV}},
-    {{"xsfvcp"}, {ImpliedExtsXSfvcp}},
-    {{"xsfvfnrclipxfqf"}, {ImpliedExtsXSfvfnrclipxfqf}},
-    {{"xsfvfwmaccqqq"}, {ImpliedExtsXSfvfwmaccqqq}},
-    {{"xsfvqmaccdod"}, {ImpliedExtsXSfvqmaccdod}},
-    {{"xsfvqmaccqoq"}, {ImpliedExtsXSfvqmaccqoq}},
-    {{"xtheadvdot"}, {ImpliedExtsXTHeadVdot}},
-    {{"zacas"}, {ImpliedExtsZacas}},
-    {{"zcb"}, {ImpliedExtsZcb}},
-    {{"zcd"}, {ImpliedExtsZcd}},
-    {{"zce"}, {ImpliedExtsZce}},
-    {{"zcf"}, {ImpliedExtsZcf}},
-    {{"zcmop"}, {ImpliedExtsZcmop}},
-    {{"zcmp"}, {ImpliedExtsZcmp}},
-    {{"zcmt"}, {ImpliedExtsZcmt}},
-    {{"zdinx"}, {ImpliedExtsZdinx}},
-    {{"zfa"}, {ImpliedExtsZfa}},
-    {{"zfbfmin"}, {ImpliedExtsZfbfmin}},
-    {{"zfh"}, {ImpliedExtsZfh}},
-    {{"zfhmin"}, {ImpliedExtsZfhmin}},
-    {{"zfinx"}, {ImpliedExtsZfinx}},
-    {{"zhinx"}, {ImpliedExtsZhinx}},
-    {{"zhinxmin"}, {ImpliedExtsZhinxmin}},
-    {{"zicfiss"}, {ImpliedExtsZicfiss}},
-    {{"zicntr"}, {ImpliedExtsZicntr}},
-    {{"zihpm"}, {ImpliedExtsZihpm}},
-    {{"zk"}, {ImpliedExtsZk}},
-    {{"zkn"}, {ImpliedExtsZkn}},
-    {{"zks"}, {ImpliedExtsZks}},
-    {{"zvbb"}, {ImpliedExtsZvbb}},
-    {{"zve32f"}, {ImpliedExtsZve32f}},
-    {{"zve32x"}, {ImpliedExtsZve32x}},
-    {{"zve64d"}, {ImpliedExtsZve64d}},
-    {{"zve64f"}, {ImpliedExtsZve64f}},
-    {{"zve64x"}, {ImpliedExtsZve64x}},
-    {{"zvfbfmin"}, {ImpliedExtsZvfbfmin}},
-    {{"zvfbfwma"}, {ImpliedExtsZvfbfwma}},
-    {{"zvfh"}, {ImpliedExtsZvfh}},
-    {{"zvfhmin"}, {ImpliedExtsZvfhmin}},
-    {{"zvkn"}, {ImpliedExtsZvkn}},
-    {{"zvknc"}, {ImpliedExtsZvknc}},
-    {{"zvkng"}, {ImpliedExtsZvkng}},
-    {{"zvknhb"}, {ImpliedExtsZvknhb}},
-    {{"zvks"}, {ImpliedExtsZvks}},
-    {{"zvksc"}, {ImpliedExtsZvksc}},
-    {{"zvksg"}, {ImpliedExtsZvksg}},
-    {{"zvl1024b"}, {ImpliedExtsZvl1024b}},
-    {{"zvl128b"}, {ImpliedExtsZvl128b}},
-    {{"zvl16384b"}, {ImpliedExtsZvl16384b}},
-    {{"zvl2048b"}, {ImpliedExtsZvl2048b}},
-    {{"zvl256b"}, {ImpliedExtsZvl256b}},
-    {{"zvl32768b"}, {ImpliedExtsZvl32768b}},
-    {{"zvl4096b"}, {ImpliedExtsZvl4096b}},
-    {{"zvl512b"}, {ImpliedExtsZvl512b}},
-    {{"zvl64b"}, {ImpliedExtsZvl64b}},
-    {{"zvl65536b"}, {ImpliedExtsZvl65536b}},
-    {{"zvl8192b"}, {ImpliedExtsZvl8192b}},
+    {"d", {ImpliedExtsD}},
+    {"f", {ImpliedExtsF}},
+    {"v", {ImpliedExtsV}},
+    {"xsfvcp", {ImpliedExtsXSfvcp}},
+    {"xsfvfnrclipxfqf", {ImpliedExtsXSfvfnrclipxfqf}},
+    {"xsfvfwmaccqqq", {ImpliedExtsXSfvfwmaccqqq}},
+    {"xsfvqmaccdod", {ImpliedExtsXSfvqmaccdod}},
+    {"xsfvqmaccqoq", {ImpliedExtsXSfvqmaccqoq}},
+    {"xtheadvdot", {ImpliedExtsXTHeadVdot}},
+    {"zacas", {ImpliedExtsZacas}},
+    {"zcb", {ImpliedExtsZcb}},
+    {"zcd", {ImpliedExtsZcd}},
+    {"zce", {ImpliedExtsZce}},
+    {"zcf", {ImpliedExtsZcf}},
+    {"zcmop", {ImpliedExtsZcmop}},
+    {"zcmp", {ImpliedExtsZcmp}},
+    {"zcmt", {ImpliedExtsZcmt}},
+    {"zdinx", {ImpliedExtsZdinx}},
+    {"zfa", {ImpliedExtsZfa}},
+    {"zfbfmin", {ImpliedExtsZfbfmin}},
+    {"zfh", {ImpliedExtsZfh}},
+    {"zfhmin", {ImpliedExtsZfhmin}},
+    {"zfinx", {ImpliedExtsZfinx}},
+    {"zhinx", {ImpliedExtsZhinx}},
+    {"zhinxmin", {ImpliedExtsZhinxmin}},
+    {"zicfiss", {ImpliedExtsZicfiss}},
+    {"zicntr", {ImpliedExtsZicntr}},
+    {"zihpm", {ImpliedExtsZihpm}},
+    {"zk", {ImpliedExtsZk}},
+    {"zkn", {ImpliedExtsZkn}},
+    {"zks", {ImpliedExtsZks}},
+    {"zvbb", {ImpliedExtsZvbb}},
+    {"zve32f", {ImpliedExtsZve32f}},
+    {"zve32x", {ImpliedExtsZve32x}},
+    {"zve64d", {ImpliedExtsZve64d}},
+    {"zve64f", {ImpliedExtsZve64f}},
+    {"zve64x", {ImpliedExtsZve64x}},
+    {"zvfbfmin", {ImpliedExtsZvfbfmin}},
+    {"zvfbfwma", {ImpliedExtsZvfbfwma}},
+    {"zvfh", {ImpliedExtsZvfh}},
+    {"zvfhmin", {ImpliedExtsZvfhmin}},
+    {"zvkn", {ImpliedExtsZvkn}},
+    {"zvknc", {ImpliedExtsZvknc}},
+    {"zvkng", {ImpliedExtsZvkng}},
+    {"zvknhb", {ImpliedExtsZvknhb}},
+    {"zvks", {ImpliedExtsZvks}},
+    {"zvksc", {ImpliedExtsZvksc}},
+    {"zvksg", {ImpliedExtsZvksg}},
+    {"zvl1024b", {ImpliedExtsZvl1024b}},
+    {"zvl128b", {ImpliedExtsZvl128b}},
+    {"zvl16384b", {ImpliedExtsZvl16384b}},
+    {"zvl2048b", {ImpliedExtsZvl2048b}},
+    {"zvl256b", {ImpliedExtsZvl256b}},
+    {"zvl32768b", {ImpliedExtsZvl32768b}},
+    {"zvl4096b", {ImpliedExtsZvl4096b}},
+    {"zvl512b", {ImpliedExtsZvl512b}},
+    {"zvl64b", {ImpliedExtsZvl64b}},
+    {"zvl65536b", {ImpliedExtsZvl65536b}},
+    {"zvl8192b", {ImpliedExtsZvl8192b}},
 };
 
+static_assert(isSorted(ImpliedExts) &&
+              "Implied extensions are not sorted by name");
+
 void RISCVISAInfo::updateImplication() {
   bool HasE = Exts.count("e") != 0;
   bool HasI = Exts.count("i") != 0;
@@ -1146,8 +1146,6 @@ void RISCVISAInfo::updateImplication() {
     addExtension("i", Version->Major, Version->Minor);
   }
 
-  assert(llvm::is_sorted(ImpliedExts) && "Table not sorted by Name");
-
   // This loop may execute over 1 iteration since implication can be layered
   // Exits loop if no more implication is applied
   SmallSetVector<StringRef, 16> WorkList;

[asb]

I have a slight concern that one of the compilers that LLVM is meant to be buildable by is unhappy with the use of constexpr. Sadly MSVC 16.7 (apparently the oldest supported version) doesn't seem to be available on godbolt though :/

[lukel97]

Force pushed to retrigger CI

[lukel97]

I have a slight concern that one of the compilers that LLVM is meant to be buildable by is unhappy with the use of constexpr. Sadly MSVC 16.7 (apparently the oldest supported version) doesn't seem to be available on godbolt though :/

Yes, we ran into some MSVC specific issues before in #66199, where the constexpr was too complicated and exceeded a step limit.
The windows CI seems to have fallen over for some MLIR related build failure, so I've just restarted it there. But it looks to be using MSVC 16.11?

[topperc]

Does this give a good error message when it fails? I've inserted a print in the runtime code more than once due to bad merge resolution in my downstream to figure out which extension is misplaced.

[topperc]

Does this have compile time impact? Now we have to do a strlen call every time we search this table.

[lukel97]

I changed this because StringLiteral didn't have a constexpr < operator. But I took another stab at this, and I think we can actually make the StringLiteral std::string_view operator constexpr.

[lukel97]

Does this give a good error message when it fails? I've inserted a print in the runtime code more than once due to bad merge resolution in my downstream to figure out which extension is misplaced.

Just the same message that the current runtime check gives, e.g. "blah is not sorted". It would be nice if static_assert could actually print out the failing value or report a non-literal string, but it looks like we won't be able to until C++23.

I guess as a workaround you could comment out the static_assert when it fails, then call isSorted at runtime and add an assert.

[topperc]

I kind of think we should move the extension lists to tblgen so no one has to sort this table manually. We can probably optimize the string lengths better that way by storing them in a big array with sizes like we do in the asm matcher mnemonic table.

[lukel97]

I kind of think we should move the extension lists to tblgen so no one has to sort this table manually. We can probably optimize the string lengths better that way by storing them in a big array with sizes like we do in the asm matcher mnemonic table.

That makes sense, I would be more in favour of that approach.