[Subtarget] Remove static global constructor call from the tablegened subtarget feature tables

[craig.topper]

Summary

Subtarget features are stored in a std::bitset that has been subclassed. There is a special constructor to allow the tablegen files to provide a list of bits to initialize the std::bitset to. This constructor isn't constexpr and std::bitset doesn't support many constexpr operations either. This results in a static global constructor being used to initialize the feature bitsets in these files at startup.

To fix this I've introduced a new FeatureBitArray class that holds three 64-bit values representing the initial bit values and taught tablegen to emit hex constants for them based on the feature enum values. This makes the tablegen files less readable than they were before. I can add the list of features back as a comment if we think that's important.

I've added a method to convert from this class into the std::bitset subclass we had before. I considered making the new FeatureBitArray class just implement the std::bitset interface we need instead, but thought I'd see how others felts about that first.

I've simplified the interfaces to SetImpliedBits and ClearImpliedBits a little minimize the number of times we need to convert to the bitset.

This removes about 27K from my local release+asserts build of llc.

I'm open to any suggestions on better ways to do this.

[craig.topper]

Ping

[dblaikie]

utils/TableGen/SubtargetEmitter.cpp

179 ↗

(On Diff #187820)

Perhaps either use Mask.length() as the loop bound (or a range-based for loop) and/or change the parameter to be a reference to an array of MAX_SUBTARGET_WORDS length? (rather than having an implicit contract where you must pass this function an array of that length, but nothing checks that)] & would just walk off the end of the array otherwise)

273–277 ↗

(On Diff #187820)

This code looks like the same code over ImpliesMask above? (except for ImpliesMask rather than FeatureList) - perhaps it could be folded into the printFeatureMask function?

[bjope]

llvm/trunk/utils/TableGen/SubtargetEmitter.cpp

185

@craig.topper, I see lots of warning when compiling using clang 3.6 after this patch:

lib/Target/AArch64/AArch64GenSubtargetInfo.inc:167:63: error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces]
  { "a35", "Cortex-A35 ARM processors", AArch64::ProcA35, { { 0x20800080800800ULL, 0x0ULL, 0x0ULL, } } },
                                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                               {                                  }

Adding one more level of braces "{ { { 0x20800080800800ULL, 0x0ULL, 0x0ULL, } } }" seems to work, but would that mean that we no longer run the new constexpr ctor?

[bjope]

llvm/trunk/utils/TableGen/SubtargetEmitter.cpp

185

Ok, now I've read this: https://en.cppreference.com/w/cpp/container/array

So as described in the example

std::array<int, 3> a1{ {1, 2, 3} }; // double-braces required in C++11 prior to the CWG 1270 revision
                                    // (not needed in C++11 after the revision and in C++14 and beyond)

I guess my old clang 3.6 compiler doesn't include CWG 1270 then.

Afaict clang 3.6 should still be supported (even if I don't know how many warnings that are accepted when building).
Should we simply generate the double braces here, to work around these warnings?

[craig.topper]

@bjope , thanks! I'll add another set of curly braces.

llvm/trunk/include/llvm/MC/SubtargetFeature.h

 /// that should be used on that chip.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_MC_SUBTARGETFEATURE_H
 #define LLVM_MC_SUBTARGETFEATURE_H
 
 #include "llvm/ADT/StringRef.h"
+#include <array>
 #include <bitset>
 #include <initializer_list>
 #include <string>
 #include <vector>
 
 namespace llvm {
 
 template <typename T> class ArrayRef;
 class raw_ostream;
 class Triple;
 
-const unsigned MAX_SUBTARGET_FEATURES = 192;
+const unsigned MAX_SUBTARGET_WORDS = 3;
+const unsigned MAX_SUBTARGET_FEATURES = MAX_SUBTARGET_WORDS * 64;
+
 /// Container class for subtarget features.
 /// This is convenient because std::bitset does not have a constructor
 /// with an initializer list of set bits.
 class FeatureBitset : public std::bitset<MAX_SUBTARGET_FEATURES> {
 public:
   // Cannot inherit constructors because it's not supported by VC++..
   FeatureBitset() = default;
 
   FeatureBitset(const bitset<MAX_SUBTARGET_FEATURES>& B) : bitset(B) {}
 
   FeatureBitset(std::initializer_list<unsigned> Init) {
     for (auto I : Init)
       set(I);
   }
 };
 
+/// Class used to store the subtarget bits in the tables created by tablegen.
+/// The std::initializer_list constructor of FeatureBitset can't be done at
+/// compile time and requires a static constructor to run at startup.
+class FeatureBitArray {
+  std::array<uint64_t, MAX_SUBTARGET_WORDS> Bits;
+
+public:
+  constexpr FeatureBitArray(const std::array<uint64_t, MAX_SUBTARGET_WORDS> &B)
+      : Bits(B) {}
+
+  FeatureBitset getAsBitset() const {
+    FeatureBitset Result;
+
+    for (unsigned i = 0, e = Bits.size(); i != e; ++i)
+      Result |= FeatureBitset(Bits[i]) << (64 * i);
+
+    return Result;
+  }
+};
+
 //===----------------------------------------------------------------------===//
 
 /// Used to provide key value pairs for feature and CPU bit flags.
 struct SubtargetFeatureKV {
   const char *Key;                      ///< K-V key string
   const char *Desc;                     ///< Help descriptor
   unsigned Value;                       ///< K-V integer value
-  FeatureBitset Implies;                ///< K-V bit mask
+  FeatureBitArray Implies;              ///< K-V bit mask
 
   /// Compare routine for std::lower_bound
   bool operator<(StringRef S) const {
     return StringRef(Key) < S;
   }
 
   /// Compare routine for std::is_sorted.
   bool operator<(const SubtargetFeatureKV &Other) const {

llvm/trunk/lib/MC/SubtargetFeature.cpp

 }
 
 std::string SubtargetFeatures::getString() const {
   return join(Features.begin(), Features.end(), ",");
 }
 
 /// For each feature that is (transitively) implied by this feature, set it.
 static
-void SetImpliedBits(FeatureBitset &Bits, const SubtargetFeatureKV &FeatureEntry,
+void SetImpliedBits(FeatureBitset &Bits, const FeatureBitset &Implies,
                     ArrayRef<SubtargetFeatureKV> FeatureTable) {
   for (const SubtargetFeatureKV &FE : FeatureTable) {
-    if (FeatureEntry.Value == FE.Value) continue;
-
-    if (FeatureEntry.Implies.test(FE.Value)) {
+    if (Implies.test(FE.Value)) {
       Bits.set(FE.Value);
-      SetImpliedBits(Bits, FE, FeatureTable);
+      SetImpliedBits(Bits, FE.Implies.getAsBitset(), FeatureTable);
     }
   }
 }
 
 /// For each feature that (transitively) implies this feature, clear it.
 static
-void ClearImpliedBits(FeatureBitset &Bits,
-                      const SubtargetFeatureKV &FeatureEntry,
+void ClearImpliedBits(FeatureBitset &Bits, unsigned Value,
                       ArrayRef<SubtargetFeatureKV> FeatureTable) {
   for (const SubtargetFeatureKV &FE : FeatureTable) {
-    if (FeatureEntry.Value == FE.Value) continue;
-
-    if (FE.Implies.test(FeatureEntry.Value)) {
+    if (FE.Implies.getAsBitset().test(Value)) {
       Bits.reset(FE.Value);
-      ClearImpliedBits(Bits, FE, FeatureTable);
+      ClearImpliedBits(Bits, FE.Value, FeatureTable);
     }
   }
 }
 
 void
 SubtargetFeatures::ToggleFeature(FeatureBitset &Bits, StringRef Feature,
                                  ArrayRef<SubtargetFeatureKV> FeatureTable) {
   // Find feature in table.
   const SubtargetFeatureKV *FeatureEntry =
       Find(StripFlag(Feature), FeatureTable);
   // If there is a match
   if (FeatureEntry) {
     if (Bits.test(FeatureEntry->Value)) {
       Bits.reset(FeatureEntry->Value);
       // For each feature that implies this, clear it.
-      ClearImpliedBits(Bits, *FeatureEntry, FeatureTable);
+      ClearImpliedBits(Bits, FeatureEntry->Value, FeatureTable);
     } else {
       Bits.set(FeatureEntry->Value);
 
       // For each feature that this implies, set it.
-      SetImpliedBits(Bits, *FeatureEntry, FeatureTable);
+      SetImpliedBits(Bits, FeatureEntry->Implies.getAsBitset(), FeatureTable);
     }
   } else {
     errs() << "'" << Feature << "' is not a recognized feature for this target"
            << " (ignoring feature)\n";
   }
 }
 
 void SubtargetFeatures::ApplyFeatureFlag(FeatureBitset &Bits, StringRef Feature,
                                     ArrayRef<SubtargetFeatureKV> FeatureTable) {
   assert(hasFlag(Feature));
 
   // Find feature in table.
   const SubtargetFeatureKV *FeatureEntry =
       Find(StripFlag(Feature), FeatureTable);
   // If there is a match
   if (FeatureEntry) {
     // Enable/disable feature in bits
     if (isEnabled(Feature)) {
       Bits.set(FeatureEntry->Value);
 
       // For each feature that this implies, set it.
-      SetImpliedBits(Bits, *FeatureEntry, FeatureTable);
+      SetImpliedBits(Bits, FeatureEntry->Implies.getAsBitset(), FeatureTable);
     } else {
       Bits.reset(FeatureEntry->Value);
 
       // For each feature that implies this, clear it.
-      ClearImpliedBits(Bits, *FeatureEntry, FeatureTable);
+      ClearImpliedBits(Bits, FeatureEntry->Value, FeatureTable);
     }
   } else {
     errs() << "'" << Feature << "' is not a recognized feature for this target"
            << " (ignoring feature)\n";
   }
 }
 
 FeatureBitset
SubtargetFeatures::getFeatureBits(StringRef CPU,
 
   // Find CPU entry if CPU name is specified.
   else if (!CPU.empty()) {
     const SubtargetFeatureKV *CPUEntry = Find(CPU, CPUTable);
 
     // If there is a match
     if (CPUEntry) {
       // Set base feature bits
-      Bits = CPUEntry->Implies;
+      FeatureBitset CPUImplies = CPUEntry->Implies.getAsBitset();
+      Bits = CPUImplies;
 
       // Set the feature implied by this CPU feature, if any.
       for (auto &FE : FeatureTable) {
-        if (CPUEntry->Implies.test(FE.Value))
-          SetImpliedBits(Bits, FE, FeatureTable);
+        if (CPUImplies.test(FE.Value))
+          SetImpliedBits(Bits, FE.Implies.getAsBitset(), FeatureTable);
       }
     } else {
       errs() << "'" << CPU << "' is not a recognized processor for this target"
              << " (ignoring processor)\n";
     }
   }
 
   // Iterate through each feature

llvm/trunk/utils/TableGen/SubtargetEmitter.cpp

struct LessWriteProcResources {
     }
   };
 
   const CodeGenTarget &TGT;
   RecordKeeper &Records;
   CodeGenSchedModels &SchedModels;
   std::string Target;
 
-  void Enumeration(raw_ostream &OS);
-  unsigned FeatureKeyValues(raw_ostream &OS);
-  unsigned CPUKeyValues(raw_ostream &OS);
+  void Enumeration(raw_ostream &OS, DenseMap<Record *, unsigned> &FeatureMap);
+  unsigned FeatureKeyValues(raw_ostream &OS,
+                            const DenseMap<Record *, unsigned> &FeatureMap);
+  unsigned CPUKeyValues(raw_ostream &OS,
+                        const DenseMap<Record *, unsigned> &FeatureMap);
   void FormItineraryStageString(const std::string &Names,
                                 Record *ItinData, std::string &ItinString,
                                 unsigned &NStages);
   void FormItineraryOperandCycleString(Record *ItinData, std::string &ItinString,
                                        unsigned &NOperandCycles);
   void FormItineraryBypassString(const std::string &Names,
                                  Record *ItinData,
                                  std::string &ItinString, unsigned NOperandCycles);
public:
   void run(raw_ostream &o);
 };
 
 } // end anonymous namespace
 
 //
 // Enumeration - Emit the specified class as an enumeration.
 //
-void SubtargetEmitter::Enumeration(raw_ostream &OS) {
+void SubtargetEmitter::Enumeration(raw_ostream &OS,
+                                   DenseMap<Record *, unsigned> &FeatureMap) {
   // Get all records of class and sort
   std::vector<Record*> DefList =
     Records.getAllDerivedDefinitions("SubtargetFeature");
   llvm::sort(DefList, LessRecord());
 
   unsigned N = DefList.size();
   if (N == 0)
     return;
   if (N > MAX_SUBTARGET_FEATURES)
     PrintFatalError("Too many subtarget features! Bump MAX_SUBTARGET_FEATURES.");
 
   OS << "namespace " << Target << " {\n";
 
   // Open enumeration.
   OS << "enum {\n";
 
   // For each record
   for (unsigned i = 0; i < N; ++i) {
     // Next record
     Record *Def = DefList[i];
 
     // Get and emit name
     OS << "  " << Def->getName() << " = " << i << ",\n";
+
+    // Save the index for this feature.
+    FeatureMap[Def] = i;
   }
 
   // Close enumeration and namespace
   OS << "};\n";
   OS << "} // end namespace " << Target << "\n";
 }
 
+static void printFeatureMask(raw_ostream &OS, RecVec &FeatureList,
+                             const DenseMap<Record *, unsigned> &FeatureMap) {
+  std::array<uint64_t, MAX_SUBTARGET_WORDS> Mask = {};
+  for (unsigned j = 0, M = FeatureList.size(); j < M; ++j) {
+    unsigned Bit = FeatureMap.lookup(FeatureList[j]);
+    Mask[Bit / 64] |= 1ULL << (Bit % 64);
+  }
+
+  OS << "{ { ";
+  for (unsigned i = 0; i != Mask.size(); ++i) {
+    OS << "0x";
+    OS.write_hex(Mask[i]);
+    OS << "ULL, ";
+  }
+  OS << "} }";
+}
+
 //
 // FeatureKeyValues - Emit data of all the subtarget features.  Used by the
 // command line.
 //
-unsigned SubtargetEmitter::FeatureKeyValues(raw_ostream &OS) {
+unsigned SubtargetEmitter::FeatureKeyValues(
+    raw_ostream &OS, const DenseMap<Record *, unsigned> &FeatureMap) {
   // Gather and sort all the features
   std::vector<Record*> FeatureList =
                            Records.getAllDerivedDefinitions("SubtargetFeature");
 
   if (FeatureList.empty())
     return 0;
 
   llvm::sort(FeatureList, LessRecordFieldName());
for (unsigned i = 0, N = FeatureList.size(); i < N; ++i) {
     // Emit as { "feature", "description", { featureEnum }, { i1 , i2 , ... , in } }
     OS << "  { "
        << "\"" << CommandLineName << "\", "
        << "\"" << Desc << "\", "
        << Target << "::" << Name << ", ";
 
     RecVec ImpliesList = Feature->getValueAsListOfDefs("Implies");
 
-    OS << "{";
-    for (unsigned j = 0, M = ImpliesList.size(); j < M;) {
-      OS << " " << Target << "::" << ImpliesList[j]->getName();
-      if (++j < M) OS << ",";
-    }
-    OS << " } },\n";
+    printFeatureMask(OS, ImpliesList, FeatureMap);
+
+    OS << " },\n";
     ++NumFeatures;
   }
 
   // End feature table
   OS << "};\n";
 
   return NumFeatures;
 }
 
 //
 // CPUKeyValues - Emit data of all the subtarget processors.  Used by command
 // line.
 //
-unsigned SubtargetEmitter::CPUKeyValues(raw_ostream &OS) {
+unsigned
+SubtargetEmitter::CPUKeyValues(raw_ostream &OS,
+                               const DenseMap<Record *, unsigned> &FeatureMap) {
   // Gather and sort processor information
   std::vector<Record*> ProcessorList =
                           Records.getAllDerivedDefinitions("Processor");
   llvm::sort(ProcessorList, LessRecordFieldName());
 
   // Begin processor table
   OS << "// Sorted (by key) array of values for CPU subtype.\n"
      << "extern const llvm::SubtargetFeatureKV " << Target
      << "SubTypeKV[] = {\n";
 
   // For each processor
   for (Record *Processor : ProcessorList) {
     StringRef Name = Processor->getValueAsString("Name");
     RecVec FeatureList = Processor->getValueAsListOfDefs("Features");
 
     // Emit as { "cpu", "description", 0, { f1 , f2 , ... fn } },
     // The 0 is for the feature id which isn't used for CPUs.
     OS << " { "
        << "\"" << Name << "\", "
        << "\"Select the " << Name << " processor\", 0, ";
 
-    OS << "{";
-    for (unsigned j = 0, M = FeatureList.size(); j < M;) {
-      OS << " " << Target << "::" << FeatureList[j]->getName();
-      if (++j < M) OS << ",";
-    }
-    OS << " } },\n";
+    printFeatureMask(OS, FeatureList, FeatureMap);
+
+    // The {{}} is for the "implies" section of this data structure.
+    OS << " },\n";
   }
 
   // End processor table
   OS << "};\n";
 
   return ProcessorList.size();
 }
 

 // SubtargetEmitter::run - Main subtarget enumeration emitter.
 //
 void SubtargetEmitter::run(raw_ostream &OS) {
   emitSourceFileHeader("Subtarget Enumeration Source Fragment", OS);
 
   OS << "\n#ifdef GET_SUBTARGETINFO_ENUM\n";
   OS << "#undef GET_SUBTARGETINFO_ENUM\n\n";
 
+  DenseMap<Record *, unsigned> FeatureMap;
+
   OS << "namespace llvm {\n";
-  Enumeration(OS);
+  Enumeration(OS, FeatureMap);
   OS << "} // end namespace llvm\n\n";
   OS << "#endif // GET_SUBTARGETINFO_ENUM\n\n";
 
   OS << "\n#ifdef GET_SUBTARGETINFO_MC_DESC\n";
   OS << "#undef GET_SUBTARGETINFO_MC_DESC\n\n";
 
   OS << "namespace llvm {\n";
 #if 0
   OS << "namespace {\n";
 #endif
-  unsigned NumFeatures = FeatureKeyValues(OS);
+  unsigned NumFeatures = FeatureKeyValues(OS, FeatureMap);
   OS << "\n";
-  unsigned NumProcs = CPUKeyValues(OS);
+  unsigned NumProcs = CPUKeyValues(OS, FeatureMap);
   OS << "\n";
   EmitSchedModel(OS);
   OS << "\n";
 #if 0
   OS << "} // end anonymous namespace\n\n";
 #endif
 
   // MCInstrInfo initialization routine.