{tablegen] Emit string literals instead of char arrays

[ldrumm]

Summary

[tablegen] Emit string literals instead of char arrays

This changes the generated (Instr|Asm|Reg|Regclass)Name tables from this
form:

extern const char HexagonInstrNameData[] = {
  /* 0 */ 'G', '_', 'F', 'L', 'O', 'G', '1', '0', 0,
  /* 9 */ 'E', 'N', 'D', 'L', 'O', 'O', 'P', '0', 0,
  /* 18 */ 'V', '6', '_', 'v', 'd', 'd', '0', 0,
  /* 26 */ 'P', 'S', '_', 'v', 'd', 'd', '0', 0,
  [...]
};

...to this:

extern const char HexagonInstrNameData[] = {
  /* 0 */ "G_FLOG10\0"
  /* 9 */ "ENDLOOP0\0"
  /* 18 */ "V6_vdd0\0"
  /* 26 */ "PS_vdd0\0"
  [...]
};

This should make debugging and exploration a lot easier for mortals,
while providing a significant compile-time reduction for common compilers.

To avoid issues with low implementation limits, this is disabled by
default for visual studio or when cross-compiling.

To force output one way or the other, pass
--long-string-literals=<bool> to tablegen

[mstorsjo]

utils/TableGen/CMakeLists.txt

64 ↗

(On Diff #239119)

I'm afraid this isn't sufficient; I normally first do a fully native build, then when crosscompiling, I point it at the already built fully native tblgen.

Also likewise if I don't have an already built tblgen when crosscompiling, cmake is invoked a second time recursively for building these tools, and I'd guess CMAKE_CROSSCOMPILING isn't set there, as it's a normal native build for those tools.

I think the one safe way of handling it would be to add a runtime option to tblgen for making msvc compatible output, and set that when generating tables, if the target compiler is msvc. Then it should work the same with any combination of build configurations. I don't know the tblgen internals if this is doable or what it looks like wrt the tblgen invocations in cmake though...

[ldrumm]

Ensure elaborate cross-compilation setups are supported

[rnk]

Looks good to me, but please let @mstorsjo confirm that this will work before landing. Thanks!

[mstorsjo]

cmake/modules/TableGen.cmake

68 ↗

(On Diff #239163)

I think this needs to go inside a if (MSVC) clause instead, as the generator often can be Ninja even though you're building with MSVC.

[ldrumm]

Change the cmake flag check to if(MSVC) rather than if(CMAKE_GENERATOR MATCHES "Visual Studio"). CMAKE_GENERATOR is not the compiler

[mstojanovic]

First char of the title: { -> [.

[ldrumm]

In D73044#1831436, @mstojanovic wrote:

First char of the title: { -> [.

Thanks. Phabricator doesn't seem to track the git commit message, but I've fixed this is on my tree

cmake/modules/TableGen.cmake

68 ↗

(On Diff #239163)

fixed

utils/TableGen/CMakeLists.txt

64 ↗

(On Diff #239119)

The logic to do that already includes workarounds for visual studio.
I've added them in the same place:

diff
diff --git a/cmake/modules/TableGen.cmake b/cmake/modules/TableGen.cmake
index a1c28870c14..4d888696ec7 100644
--- a/cmake/modules/TableGen.cmake
+++ b/cmake/modules/TableGen.cmake
@@ -63,8 +63,13 @@ function(tablegen project ofn)
     # behavior. Since it doesn't do restat optimizations anyway, just don't
     # pass --write-if-changed there.
     set(tblgen_change_flag)
+    # Visual Studio has problems with string literals greater than 65534 bytes
+    # in length. Ensure tablgen works around this when the generator is VS
+    set(tblgen_long_string_literals_flag "--long-string-literals=0")
   else()
     set(tblgen_change_flag "--write-if-changed")
+    # otherwise use the configuration default
+    set(tblgen_long_string_literals_flag)
   endif()
 
   # We need both _TABLEGEN_TARGET and _TABLEGEN_EXE in the  DEPENDS list
@@ -81,6 +86,7 @@ function(tablegen project ofn)
     ${LLVM_TABLEGEN_FLAGS}
     ${LLVM_TARGET_DEFINITIONS_ABSOLUTE}
     ${tblgen_change_flag}
+    ${tblgen_long_string_literals_flag}
     ${additional_cmdline}
     # The file in LLVM_TARGET_DEFINITIONS may be not in the current
     # directory and local_tds may not contain it, so we must

[mstorsjo]

utils/TableGen/CMakeLists.txt

64 ↗

(On Diff #239272)

There's no need to check the crosscompiling flag any longer. If the compiler that will compile the generated code is msvc (be it cross or not), we need the legacy mode, but if crosscompiling with something else, we can generate the more efficient code.

I would suggest dropping this bit of the change altogether (and the tablegen-config.h file). The parameter when invoking the tool should always know precisely whether it's needed or not.

[ldrumm]

Remove compile-time detection. Make this a runtime only flag

[mstorsjo]

Awesome, this looks fine with me. I haven't tested it, but it looks sensible to me. I haven't looked at the implementation itself though, but @rnk already approved an earlier version wrt that.

[broadwaylamb]

Hello @ldrumm,

Sorry, I had to revert this commit in e464b31c1565204e3be114d043bcbf4de61fe2e9, because it broke some Windows builds:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/13870

[ldrumm]

In D73044#1836244, @broadwaylamb wrote:

Hello @ldrumm,

Sorry, I had to revert this commit in e464b31c1565204e3be114d043bcbf4de61fe2e9, because it broke some Windows builds:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/13870

Understood.
It looks like tablegen command-line options are dotted over llvm/lib/Tablegen and utils/Tablegen and the clang tablegen binary just links against the llvm/lib/Tablegen code, but as I only built llvm, not clang the tests all passed for me. Given this option can only affect the llvm-tablegen utility (it's use is in a private header of the llvm-tablegen source tree), it seems like we need to detect this with cmake. I'll update the patch.

[kparzysz]

There is also the issue of warnings about the size of the generated string. This would cause -Werror builds to fail.

In file included from /w/src/llvm.org/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:44:
/w/bld/org/lib/Target/X86/X86GenInstrInfo.inc:32952:11: warning: string literal of length 209447 exceeds maximum length 65536 that C++ compilers are required to support [-Woverlength-strings]
  /* 0 */ "G_FLOG10\0"
          ^~~~~~~~~~~~

[ldrumm]

• only pass the --long-string-literals flags to *LLVM* tablegen (fixes clang-tablegen problem)
• suppress overlenth-string-literals warning seen in the wild: this behaviour is intended and shouldn't be warned against

[rnk]

llvm/cmake/modules/HandleLLVMOptions.cmake

652 ↗

(On Diff #239925)

Instead of this, I would suggest doing this in the generated source:

#ifdef __GNUC
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Woverlength-strings"
#endif
...
#ifdef __GNUC
#pragma GCC diagnostic pop
#endif

That way, we still benefit from this warning in places where we aren't explicitly taking steps to avoid the problem.

[rnk]

llvm/cmake/modules/TableGen.cmake

66

This works for now, but I just recently I wanted to use SequenceToOffsetTable in clang, but I used StringToOffsetTable.h instead. So far our such tables are small enough not to cause problems, but eventually we probably want to move this thing to the TableGen library, etc, etc. Anyway, this is not a blocking concern.
f63d7637387995765e9ece0e10fe1b5a4f0612b5

[ldrumm]

llvm/cmake/modules/HandleLLVMOptions.cmake

652 ↗

(On Diff #239925)

While limiting the surface of the warning might be nice, I'm not convinced by the value proposition of adding code to emit this in every tablegen backend.
I could add emit_boilerplate_macros_prologue/epilogue() functions to SequenceToTableOffset that are called at the beginning and end of every tablegen backend, but again I don't think it's worth the extra complexity vs silencing a basically pointless warning on a compiler that has never had this limitatation

[ldrumm]

llvm/cmake/modules/TableGen.cmake

66

Sounds good. I'm happy to revisit this.

[rnk]

llvm/cmake/modules/HandleLLVMOptions.cmake

652 ↗

(On Diff #239925)

I guess I was hoping it would be enough to splat that pragma out from emit(). If we can't place the pragma inside the array with long strings, one could update the API of emit to take the type and name of the array so we can get the pragma in the right place without too much duplication. All the callers use the same code pattern anyway, if you ignore the (IMO unnecessary) std::array usage:
https://reviews.llvm.org/P8187

[ldrumm]

Now emit long-string disabling GCC diagnostic pragmas around string definitions to silence gcc's -Woverlength-strings on a case-by-case basis

[ldrumm]

llvm/cmake/modules/HandleLLVMOptions.cmake

652 ↗

(On Diff #239925)

Okay. I understand. We can't have the pragma *inside* the initializer (hard error), so we have to take the declaration name from the user code and emit that after the pragma a'la:

#ifdef __GNUC__
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Woverlength-strings"
#endif
extern const char AArch64RegStrings[] = {
  /* 0 */ "B10\0"
 [...]
  /* 2397 */ "NZCV\0"
};
#ifdef __GNUC__
#pragma GCC diagnostic pop
#endif

and the api for this now looks like this in use:

c++
  // Emit the string table.
  RegStrings.layout();
  RegStrings.emitStringLiteralDef(
      OS, Twine("extern const char ") + TargetName + "RegStrings[]");

[rnk]

Thanks!

llvm/cmake/modules/TableGen.cmake

function(tablegen project ofn)
   if (LLVM_ENABLE_GISEL_COV)
     list(FIND ARGN "-gen-global-isel" idx)
     if( NOT idx EQUAL -1 )
       list(APPEND LLVM_TABLEGEN_FLAGS "-instrument-gisel-coverage")
       list(APPEND LLVM_TABLEGEN_FLAGS "-gisel-coverage-file=${LLVM_GISEL_COV_PREFIX}all")
     endif()
   endif()
 
+  # MSVC can't support long string literals ("long" > 65534 bytes)[1], so if there's
+  # a possibility of generated tables being consumed by MSVC, generate arrays of
+  # char literals, instead. If we're cross-compiling, then conservatively assume
+  # that the source might be consumed by MSVC.
+  # [1] https://docs.microsoft.com/en-us/cpp/cpp/compiler-limits?view=vs-2017
+  if (MSVC AND project STREQUAL LLVM)
+    list(APPEND LLVM_TABLEGEN_FLAGS "--long-string-literals=0")
+  endif()
   if (CMAKE_GENERATOR MATCHES "Visual Studio")
     # Visual Studio has problems with llvm-tblgen's native --write-if-changed
     # behavior. Since it doesn't do restat optimizations anyway, just don't
     # pass --write-if-changed there.
     set(tblgen_change_flag)
   else()
     set(tblgen_change_flag "--write-if-changed")
   endif()

llvm/utils/TableGen/AsmWriterEmitter.cpp

while (true) {
     }
     BitsLeft -= NumBits;
 
     // Remember the handlers for this set of operands.
     TableDrivenOperandPrinters.push_back(std::move(UniqueOperandCommands));
   }
 
   // Emit the string table itself.
-  O << "  static const char AsmStrs[] = {\n";
-  StringTable.emit(O, printChar);
-  O << "  };\n\n";
+  StringTable.emitStringLiteralDef(O, "  static const char AsmStrs[]");
 
   // Emit the lookup tables in pieces to minimize wasted bytes.
   unsigned BytesNeeded = ((OpcodeInfoBits - BitsLeft) + 7) / 8;
   unsigned Table = 0, Shift = 0;
   SmallString<128> BitsString;
   raw_svector_ostream BitsOS(BitsString);
   // If the total bits is more than 32-bits we need to use a 64-bit type.
   BitsOS << "  uint" << ((BitsLeft < (OpcodeInfoBits - 32)) ? 64 : 32)
if (AltName == "" || AltName == "NoRegAltName") {
                           AltName + "'.");
         AsmName = AltNames[Idx];
       }
     }
     StringTable.add(AsmName);
   }
 
   StringTable.layout();
-  O << "  static const char AsmStrs" << AltName << "[] = {\n";
-  StringTable.emit(O, printChar);
-  O << "  };\n\n";
+  StringTable.emitStringLiteralDef(O, Twine("  static const char AsmStrs") +
+                                          AltName + "[]");
 
   O << "  static const " << getMinimalTypeForRange(StringTable.size() - 1, 32)
     << " RegAsmOffset" << AltName << "[] = {";
   for (unsigned i = 0, e = Registers.size(); i != e; ++i) {
     if ((i % 14) == 0)
       O << "\n    ";
     O << StringTable.get(AsmNames[i]) << ", ";
   }

llvm/utils/TableGen/InstrInfoEmitter.cpp

for (const CodeGenInstruction *Inst : NumberedInstructions) {
     InstrNames.add(Inst->TheDef->getName());
     // Emit the record into the table.
     emitRecord(*Inst, Num++, InstrInfo, EmittedLists, OperandInfoIDs, OS);
   }
   OS << "};\n\n";
 
   // Emit the array of instruction names.
   InstrNames.layout();
-  OS << "extern const char " << TargetName << "InstrNameData[] = {\n";
-  InstrNames.emit(OS, printChar);
-  OS << "};\n\n";
+  InstrNames.emitStringLiteralDef(OS, Twine("extern const char ") + TargetName +
+                                          "InstrNameData[]");
 
   OS << "extern const unsigned " << TargetName <<"InstrNameIndices[] = {";
   Num = 0;
   for (const CodeGenInstruction *Inst : NumberedInstructions) {
     // Newline every eight entries.
     if (Num % 8 == 0)
       OS << "\n    ";
     OS << InstrNames.get(Inst->TheDef->getName()) << "U, ";

llvm/utils/TableGen/RegisterInfoEmitter.cpp

#endif
   for (const auto &Idx : SubRegIndices) {
     OS << "  { " << Idx.Offset << ", " << Idx.Size << " },\t// "
        << Idx.getName() << "\n";
   }
   OS << "};\n\n";
 
   // Emit the string table.
   RegStrings.layout();
-  OS << "extern const char " << TargetName << "RegStrings[] = {\n";
-  RegStrings.emit(OS, printChar);
-  OS << "};\n\n";
+  RegStrings.emitStringLiteralDef(OS, Twine("extern const char ") + TargetName +
+                                          "RegStrings[]");
 
   OS << "extern const MCRegisterDesc " << TargetName
      << "RegDesc[] = { // Descriptors\n";
   OS << "  { " << RegStrings.get("") << ", 0, 0, 0, 0, 0 },\n";
 
   // Emit the register descriptors now.
   i = 0;
   for (const auto &Reg : Regs) {
for (const auto &RC : RegisterClasses) {
     }
     BVE.print(OS);
     OS << "\n  };\n\n";
 
   }
   OS << "} // end anonymous namespace\n\n";
 
   RegClassStrings.layout();
-  OS << "extern const char " << TargetName << "RegClassStrings[] = {\n";
-  RegClassStrings.emit(OS, printChar);
-  OS << "};\n\n";
+  RegClassStrings.emitStringLiteralDef(
+      OS, Twine("extern const char ") + TargetName + "RegClassStrings[]");
 
   OS << "extern const MCRegisterClass " << TargetName
      << "MCRegisterClasses[] = {\n";
 
   for (const auto &RC : RegisterClasses) {
     assert(isInt<8>(RC.CopyCost) && "Copy cost too large.");
     OS << "  { " << RC.getName() << ", " << RC.getName() << "Bits, "
        << RegClassStrings.get(RC.getName()) << ", "

llvm/utils/TableGen/SequenceToOffsetTable.h

 // sequences as one big array.  Use the same memory when a sequence is a suffix
 // of another.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_UTILS_TABLEGEN_SEQUENCETOOFFSETTABLE_H
 #define LLVM_UTILS_TABLEGEN_SEQUENCETOOFFSETTABLE_H
 
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
 #include <cctype>
 #include <functional>
 #include <map>
 
 namespace llvm {
+extern llvm::cl::opt<bool> EmitLongStrLiterals;
+
+// Helper function for SequenceToOffsetTable<string>.
+static inline void printStrLitEscChar(raw_ostream &OS, char C) {
+  const char *Escapes[] = {
+      "\\000", "\\001", "\\002", "\\003", "\\004", "\\005", "\\006", "\\007",
+      "\\010", "\\t",   "\\n",   "\\013", "\\014", "\\r",   "\\016", "\\017",
+      "\\020", "\\021", "\\022", "\\023", "\\024", "\\025", "\\026", "\\027",
+      "\\030", "\\031", "\\032", "\\033", "\\034", "\\035", "\\036", "\\037",
+      " ",     "!",     "\\\"",  "#",     "$",     "%",     "&",     "'",
+      "(",     ")",     "*",     "+",     ",",     "-",     ".",     "/",
+      "0",     "1",     "2",     "3",     "4",     "5",     "6",     "7",
+      "8",     "9",     ":",     ";",     "<",     "=",     ">",     "?",
+      "@",     "A",     "B",     "C",     "D",     "E",     "F",     "G",
+      "H",     "I",     "J",     "K",     "L",     "M",     "N",     "O",
+      "P",     "Q",     "R",     "S",     "T",     "U",     "V",     "W",
+      "X",     "Y",     "Z",     "[",     "\\\\",  "]",     "^",     "_",
+      "`",     "a",     "b",     "c",     "d",     "e",     "f",     "g",
+      "h",     "i",     "j",     "k",     "l",     "m",     "n",     "o",
+      "p",     "q",     "r",     "s",     "t",     "u",     "v",     "w",
+      "x",     "y",     "z",     "{",     "|",     "}",     "~",     "\\177",
+      "\\200", "\\201", "\\202", "\\203", "\\204", "\\205", "\\206", "\\207",
+      "\\210", "\\211", "\\212", "\\213", "\\214", "\\215", "\\216", "\\217",
+      "\\220", "\\221", "\\222", "\\223", "\\224", "\\225", "\\226", "\\227",
+      "\\230", "\\231", "\\232", "\\233", "\\234", "\\235", "\\236", "\\237",
+      "\\240", "\\241", "\\242", "\\243", "\\244", "\\245", "\\246", "\\247",
+      "\\250", "\\251", "\\252", "\\253", "\\254", "\\255", "\\256", "\\257",
+      "\\260", "\\261", "\\262", "\\263", "\\264", "\\265", "\\266", "\\267",
+      "\\270", "\\271", "\\272", "\\273", "\\274", "\\275", "\\276", "\\277",
+      "\\300", "\\301", "\\302", "\\303", "\\304", "\\305", "\\306", "\\307",
+      "\\310", "\\311", "\\312", "\\313", "\\314", "\\315", "\\316", "\\317",
+      "\\320", "\\321", "\\322", "\\323", "\\324", "\\325", "\\326", "\\327",
+      "\\330", "\\331", "\\332", "\\333", "\\334", "\\335", "\\336", "\\337",
+      "\\340", "\\341", "\\342", "\\343", "\\344", "\\345", "\\346", "\\347",
+      "\\350", "\\351", "\\352", "\\353", "\\354", "\\355", "\\356", "\\357",
+      "\\360", "\\361", "\\362", "\\363", "\\364", "\\365", "\\366", "\\367",
+      "\\370", "\\371", "\\372", "\\373", "\\374", "\\375", "\\376", "\\377"};
+
+  static_assert(sizeof Escapes / sizeof Escapes[0] ==
+                    std::numeric_limits<unsigned char>::max() + 1,
+                "unsupported character type");
+  OS << Escapes[static_cast<unsigned char>(C)];
+}
+
+static inline void printChar(raw_ostream &OS, char C) {
+  unsigned char UC(C);
+  if (isalnum(UC) || ispunct(UC)) {
+    OS << '\'';
+    if (C == '\\' || C == '\'')
+      OS << '\\';
+    OS << C << '\'';
+  } else {
+    OS << unsigned(UC);
+  }
+}
 
 /// SequenceToOffsetTable - Collect a number of terminated sequences of T.
 /// Compute the layout of a table that contains all the sequences, possibly by
 /// reusing entries.
 ///
 /// @tparam SeqT The sequence container. (vector or string).
 /// @tparam Less A stable comparator for SeqT elements.
 template<typename SeqT, typename Less = std::less<typename SeqT::value_type> >
public:
   unsigned get(const SeqT &Seq) const {
     assert(Entries && "Call layout() before get()");
     typename SeqMap::const_iterator I = Seqs.lower_bound(Seq);
     assert(I != Seqs.end() && isSuffix(Seq, I->first) &&
            "get() called with sequence that wasn't added first");
     return I->second + (I->first.size() - Seq.size());
   }
 
+  /// `emitStringLiteralDef` - Print out the table as the body of an array
+  /// initializer, where each element is a C string literal terminated by
+  /// `\0`. Falls back to emitting a comma-separated integer list if
+  /// `EmitLongStrLiterals` is false
+  void emitStringLiteralDef(raw_ostream &OS, const llvm::Twine &Decl) const {
+    assert(Entries && "Call layout() before emitStringLiteralDef()");
+    if (EmitLongStrLiterals) {
+      OS << "\n#ifdef __GNUC__\n"
+         << "#pragma GCC diagnostic push\n"
+         << "#pragma GCC diagnostic ignored \"-Woverlength-strings\"\n"
+         << "#endif\n"
+         << Decl << " = {\n";
+    } else {
+      OS << Decl << " = {\n";
+      emit(OS, printChar, "0");
+      OS << "\n};\n\n";
+      return;
+    }
+    for (auto I : Seqs) {
+      OS << "  /* " << I.second << " */ \"";
+      for (auto C : I.first) {
+        printStrLitEscChar(OS, C);
+      }
+      OS << "\\0\"\n";
+    }
+    OS << "};\n"
+       << "#ifdef __GNUC__\n"
+       << "#pragma GCC diagnostic pop\n"
+       << "#endif\n\n";
+  }
+
   /// emit - Print out the table as the body of an array initializer.
   /// Use the Print function to print elements.
   void emit(raw_ostream &OS,
             void (*Print)(raw_ostream&, ElemT),
             const char *Term = "0") const {
     assert((empty() || Entries) && "Call layout() before emit()");
     for (typename SeqMap::const_iterator I = Seqs.begin(), E = Seqs.end();
          I != E; ++I) {
       OS << "  /* " << I->second << " */ ";
       for (typename SeqT::const_iterator SI = I->first.begin(),
              SE = I->first.end(); SI != SE; ++SI) {
         Print(OS, *SI);
         OS << ", ";
       }
       OS << Term << ",\n";
     }
   }
 };
 
-// Helper function for SequenceToOffsetTable<string>.
-static inline void printChar(raw_ostream &OS, char C) {
-  unsigned char UC(C);
-  if (isalnum(UC) || ispunct(UC)) {
-    OS << '\'';
-    if (C == '\\' || C == '\'')
-      OS << '\\';
-    OS << C << '\'';
-  } else {
-    OS << unsigned(UC);
-  }
-}
-
 } // end namespace llvm
 
 #endif

llvm/utils/TableGen/TableGen.cpp

enum ActionType {
   GenExegesis,
   GenAutomata,
 };
 
 namespace llvm {
 /// Storage for TimeRegionsOpt as a global so that backends aren't required to
 /// include CommandLine.h
 bool TimeRegions = false;
+cl::opt<bool> EmitLongStrLiterals(
+    "long-string-literals",
+    cl::desc("when emitting large string tables, prefer string literals over "
+             "comma-separated char literals. This can be a readability and "
+             "compile-time performance win, but upsets some compilers"),
+    cl::Hidden, cl::init(true));
 } // end namespace llvm
 
 namespace {
 cl::opt<ActionType> Action(
     cl::desc("Action to perform:"),
     cl::values(
         clEnumValN(PrintRecords, "print-records",
                    "Print all records to stdout (default)"),