[StrTable] Switch intrinsics to StringTable and workaround MSVC

Historically, the main example of *very* large string tables used the
`EmitCharArray`, but that was switched (without removing the API) in
order to consolidate on a nicer emission primitive.

While this large string table in `IntrinsicsImpl.inc` seems to compile
correctly on MSVC without the workaround this adds back to the nicer
emission path, other users have repeatedly hit this MSVC limitation.
This PR teaches the string offset table emission to look at the size of
the table and fall back to the char array emission strategy when the
table becomes too large.

The workaround does have the downside of making compile times worse for
large string tables, but that appears unavoidable until we can identify
known good MSVC versions and switch to requiring them for all LLVM
users. It also reduces searchability of the generated string table --
I looked at emitting a comment with each string but it is tricky because
the escaping rules for an inline comment are different from those of of
a string literal, and there's no real way to turn the string literal
into a comment.

This PR also switches the `IntrinsicsImpl.inc` string tables over to the
new `StringTable` runtime abstraction. I didn't want to do this until
landing the MSVC workaround in case it caused even this example to start
hitting the MSVC bug, but I wanted to switch here so that I could
simplify the API for emitting the string table. With the two different
emission strategies, its important to use a very exact syntax and that
seems better encapsulated in the API.

Follow-up patches will try to consolidate the remaining users onto the
single interface, but those at least were easy to separate into
follow-ups and keep this PR somewhat smaller.

Author: chandlerc

clang/utils/TableGen/ClangDiagnosticsEmitter.cpp

@@ -1785,8 +1785,7 @@ static void emitDiagArrays(DiagsInGroupTy &DiagsInGroup,
 /// This creates an `llvm::StringTable` of all the diagnostic group names.
 static void emitDiagGroupNames(const StringToOffsetTable &GroupNames,
                                raw_ostream &OS) {
-  GroupNames.EmitStringLiteralDef(
-      OS, "static constexpr llvm::StringTable DiagGroupNames");
+  GroupNames.EmitStringTableDef(OS, "DiagGroupNames");
   OS << "\n";
 }
 
@@ -1939,9 +1938,6 @@ void clang::EmitClangDiagGroups(const RecordKeeper &Records, raw_ostream &OS) {
   inferPedantic.compute(&DiagsInPedantic, &GroupsInPedantic);
 
   StringToOffsetTable GroupNames;
-  // Add an empty string to the table first so we can use `llvm::StringTable`.
-  // TODO: Factor this into `StringToOffsetTable`.
-  GroupNames.GetOrAddStringOffset("");
   for (const auto &[Name, Group] : DiagsInGroup) {
     GroupNames.GetOrAddStringOffset(Name);
   }

llvm/include/llvm/TableGen/StringToOffsetTable.h

@@ -27,6 +27,12 @@ class StringToOffsetTable {
   std::string AggregateString;
 
 public:
+  StringToOffsetTable() {
+    // Ensure we always put the empty string at offset zero. That lets empty
+    // initialization also be zero initialization for offsets into the table.
+    GetOrAddStringOffset("");
+  }
+
   bool empty() const { return StringOffset.empty(); }
   size_t size() const { return AggregateString.size(); }
 
@@ -51,28 +57,62 @@ class StringToOffsetTable {
     return II->second;
   }
 
-  // Emit the string using string literal concatenation, for better readability
-  // and searchability.
-  void EmitStringLiteralDef(raw_ostream &OS, const Twine &Decl,
-                            const Twine &Indent = "  ") const {
+  // Emit a string table definition with the provided name and indent.
+  //
+  // When possible, this uses string-literal concatenation to emit the string
+  // contents in a readable and searchable way. However, for (very) large string
+  // tables MSVC cannot reliably use string literals and so there we use a large
+  // character array. We still use a line oriented emission and add comments to
+  // provide searchability even in this case.
+  //
+  // The string table, and its input string contents, are always emitted as both
+  // `static` and `constexpr`. Both `Name` and (`Name` + "Storage") must be
+  // valid identifiers to declare.
+  void EmitStringTableDef(raw_ostream &OS, const Twine &Name,
+                          const Twine &Indent = "") const {
     OS << formatv(R"(
 #ifdef __GNUC__
 #pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Woverlength-strings"
 #endif
-{0}{1} = )",
-                  Indent, Decl);
+{0}static constexpr char {1}Storage[] = )",
+                  Indent, Name);
 
+    // MSVC silently miscompiles string literals longer than 64k in some
+    // circumstances. When the string table is longer, emit it as an array of
+    // character literals.
+    bool UseChars = AggregateString.size() > (64 * 1024);
+    OS << (UseChars ? "{\n" : "\n");
+
+    llvm::ListSeparator LineSep(UseChars ? ",\n" : "\n");
     for (StringRef Str : split(AggregateString, '\0')) {
-      OS << "\n" << Indent << "  \"";
-      OS.write_escaped(Str);
-      OS << "\\0\"";
+      OS << LineSep << Indent << "  ";
+      // If we can, just emit this as a string literal to be concatenated.
+      if (!UseChars) {
+        OS << "\"";
+        OS.write_escaped(Str);
+        OS << "\\0\"";
+        continue;
+      }
+
+      llvm::ListSeparator CharSep(", ");
+      for (char C : Str) {
+        OS << CharSep << "'";
+        OS.write_escaped(StringRef(&C, 1));
+        OS << "'";
+      }
+      OS << CharSep << "'\\0'";
     }
-    OS << R"(;
+    OS << LineSep << Indent << (UseChars ? "};" : "  ;");
+
+    OS << formatv(R"(
 #ifdef __GNUC__
 #pragma GCC diagnostic pop
 #endif
-)";
+
+{0}static constexpr llvm::StringTable {1} = {1}Storage;
+)",
+                  Indent, Name);
   }
 
   // Emit the string as one single string.
@@ -110,26 +150,6 @@ class StringToOffsetTable {
     }
     O << "\"";
   }
-
-  /// Emit the string using character literals. MSVC has a limitation that
-  /// string literals cannot be longer than 64K.
-  void EmitCharArray(raw_ostream &O) {
-    assert(AggregateString.find(')') == std::string::npos &&
-           "can't emit raw string with closing parens");
-    int Count = 0;
-    O << ' ';
-    for (char C : AggregateString) {
-      O << " \'";
-      O.write_escaped(StringRef(&C, 1));
-      O << "\',";
-      Count++;
-      if (Count > 14) {
-        O << "\n ";
-        Count = 0;
-      }
-    }
-    O << '\n';
-  }
 };
 
 } // end namespace llvm

llvm/lib/IR/Intrinsics.cpp

@@ -12,6 +12,7 @@
 
 #include "llvm/IR/Intrinsics.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringTable.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/IntrinsicsAArch64.h"
 #include "llvm/IR/IntrinsicsAMDGPU.h"
@@ -40,7 +41,7 @@ using namespace llvm;
 
 StringRef Intrinsic::getBaseName(ID id) {
   assert(id < num_intrinsics && "Invalid intrinsic ID!");
-  return IntrinsicNameTable + IntrinsicNameOffsetTable[id];
+  return IntrinsicNameTable[IntrinsicNameOffsetTable[id]];
 }
 
 StringRef Intrinsic::getName(ID id) {
@@ -649,20 +650,20 @@ static int lookupLLVMIntrinsicByName(ArrayRef<unsigned> NameOffsetTable,
       // `equal_range` requires the comparison to work with either side being an
       // offset or the value. Detect which kind each side is to set up the
       // compared strings.
-      const char *LHSStr;
+      StringRef LHSStr;
       if constexpr (std::is_integral_v<decltype(LHS)>) {
-        LHSStr = &IntrinsicNameTable[LHS];
+        LHSStr = IntrinsicNameTable[LHS];
       } else {
         LHSStr = LHS;
       }
-      const char *RHSStr;
+      StringRef RHSStr;
       if constexpr (std::is_integral_v<decltype(RHS)>) {
-        RHSStr = &IntrinsicNameTable[RHS];
+        RHSStr = IntrinsicNameTable[RHS];
       } else {
         RHSStr = RHS;
       }
-      return strncmp(LHSStr + CmpStart, RHSStr + CmpStart, CmpEnd - CmpStart) <
-             0;
+      return strncmp(LHSStr.data() + CmpStart, RHSStr.data() + CmpStart,
+                     CmpEnd - CmpStart) < 0;
     };
     LastLow = Low;
     std::tie(Low, High) = std::equal_range(Low, High, Name.data(), Cmp);
@@ -672,7 +673,7 @@ static int lookupLLVMIntrinsicByName(ArrayRef<unsigned> NameOffsetTable,
 
   if (LastLow == NameOffsetTable.end())
     return -1;
-  StringRef NameFound = &IntrinsicNameTable[*LastLow];
+  StringRef NameFound = IntrinsicNameTable[*LastLow];
   if (Name == NameFound ||
       (Name.starts_with(NameFound) && Name[NameFound.size()] == '.'))
     return LastLow - NameOffsetTable.begin();
@@ -716,7 +717,7 @@ Intrinsic::ID Intrinsic::lookupIntrinsicID(StringRef Name) {
 
   // If the intrinsic is not overloaded, require an exact match. If it is
   // overloaded, require either exact or prefix match.
-  const auto MatchSize = strlen(&IntrinsicNameTable[NameOffsetTable[Idx]]);
+  const auto MatchSize = IntrinsicNameTable[NameOffsetTable[Idx]].size();
   assert(Name.size() >= MatchSize && "Expected either exact or prefix match");
   bool IsExactMatch = Name.size() == MatchSize;
   return IsExactMatch || Intrinsic::isOverloaded(ID) ? ID

llvm/test/TableGen/MixedCasedMnemonic.td

@@ -41,7 +41,7 @@ def :MnemonicAlias<"InstB", "BInst">;
 
 // Check that the matcher lower()s the mnemonics it matches.
 // MATCHER: static const char MnemonicTable[] =
-// MATCHER-NEXT: "\005ainst\005binst";
+// MATCHER-NEXT: "\000\005ainst\005binst";
 
 // Check that aInst appears before BInst in the match table.
 // This shows that the mnemonics are sorted in a case-insensitive way,

llvm/utils/TableGen/Basic/IntrinsicEmitter.cpp

@@ -252,8 +252,7 @@ void IntrinsicEmitter::EmitIntrinsicToNameTable(
 
 )";
 
-  Table.EmitStringLiteralDef(OS, "static constexpr char IntrinsicNameTable[]",
-                             /*Indent=*/"");
+  Table.EmitStringTableDef(OS, "IntrinsicNameTable", /*Indent=*/"");
 
   OS << R"(
 static constexpr unsigned IntrinsicNameOffsetTable[] = {
@@ -759,13 +758,13 @@ Intrinsic::getIntrinsicFor{}Builtin(StringRef TargetPrefix,
   }
 
   if (!Table.empty()) {
-    Table.EmitStringLiteralDef(OS, "static constexpr char BuiltinNames[]");
+    Table.EmitStringTableDef(OS, "BuiltinNames");
 
     OS << R"(
   struct BuiltinEntry {
     ID IntrinsicID;
     unsigned StrTabOffset;
-    const char *getName() const { return &BuiltinNames[StrTabOffset]; }
+    const char *getName() const { return BuiltinNames[StrTabOffset].data(); }
     bool operator<(StringRef RHS) const {
       return strncmp(getName(), RHS.data(), RHS.size()) < 0;
     }

llvm/utils/TableGen/OptionParserEmitter.cpp

@@ -287,10 +287,6 @@ static void emitOptionParser(const RecordKeeper &Records, raw_ostream &OS) {
   array_pod_sort(PrefixesUnion.begin(), PrefixesUnion.end());
 
   llvm::StringToOffsetTable Table;
-  // Make sure the empty string is the zero-th one in the table. This both makes
-  // it easy to check for empty strings (zero offset == empty) and makes
-  // initialization cheaper for empty strings.
-  Table.GetOrAddStringOffset("");
   // We can add all the prefixes via the union.
   for (const auto &Prefix : PrefixesUnion)
     Table.GetOrAddStringOffset(Prefix);
@@ -303,9 +299,7 @@ static void emitOptionParser(const RecordKeeper &Records, raw_ostream &OS) {
   OS << "/////////\n";
   OS << "// String table\n\n";
   OS << "#ifdef OPTTABLE_STR_TABLE_CODE\n";
-  Table.EmitStringLiteralDef(
-      OS, "static constexpr llvm::StringTable OptionStrTable",
-      /*Indent=*/"");
+  Table.EmitStringTableDef(OS, "OptionStrTable", /*Indent=*/"");
   OS << "#endif // OPTTABLE_STR_TABLE_CODE\n\n";
 
   // Dump prefixes.