[PATCH] D59002: Use bitset for assembler predicates

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: lib/Target/ARM/AsmParser/ARMAsmParser.cpp:10484
       // Don't report the same set of features twice.
-      if (FeatureMissesSeen.count(MissingFeatures))
+      if (FeatureMissesSeen.count(MissingFeatures.to_ullong()))
         break;
----------------
This seems to be making the assumption that ARM feature bits fit in a unisgned long long?


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:2834
   OS << "  struct OperandMatchEntry {\n";
-  OS << "    " << getMinimalTypeForEnumBitfield(Info.SubtargetFeatures.size())
-               << " RequiredFeatures;\n";
+  OS << "    " << "unsigned RequiredFeaturesIdx;\n";
   OS << "    " << getMinimalTypeForRange(MaxMnemonicIndex)
----------------
Can we bound this to less than a full unsigned?


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:3366
+    OS << "  {";
+    for (const auto &Feature : FeatureBitset) {
+      const auto &I = Info.SubtargetFeatures.find(Feature);
----------------
How many entries end up in this table on different targets? Since we're tablegening this, can we use hex contants and use FeatureBitArray instead of invoking the std::initializer_list constructor? That probably depends on how many entries end up in the table.


================
Comment at: utils/TableGen/AsmMatcherEmitter.cpp:3392
                << " ConvertFn;\n";
-  OS << "    " << getMinimalTypeForEnumBitfield(Info.SubtargetFeatures.size())
-               << " RequiredFeatures;\n";
+  OS << "    " << "unsigned RequiredFeaturesIdx;\n";
   OS << "    " << getMinimalTypeForRange(
----------------
Surely we can bound this to less than a full unsigned? X86 uses almost no features in the assembler which means we can probably use a uint8_t and MatchEntry is used about 28000 times. So that's going to save like 3 * 28000 bytes just from X86.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59002/new/

https://reviews.llvm.org/D59002