[TableGen] Fix excessive compile time issue in FixedLenDecoderEmitter

[foad]

Summary

This patch reduces the time taken for clang to compile the generated
disassembler for an out-of-tree target with InsnType bigger than 64 bits
from 4m30s to 48s.

D67686 did a similar thing for CodeEmitterGen.

The idea is to tweak the API of the APInt-like InsnType class so that
we don't need so many temporary InsnTypes. This takes advantage of the
rule stated in D52100 that currently "no string of bits extracted
from the encoding may exceeed 64-bits", so we can use uint64_t for some
temporaries.

D52100 goes on to say that "fields are still permitted to exceed 64-bits
so long as they aren't one contiguous string of bits". This patch breaks
that by always using a "uint64_t tmp" in the generated decodeToMCInst,
but it should be easy to fix in FilterChooser::emitBinaryParser by
choosing to use a different type of tmp based on the known total field
width.

[foad]

D52100 goes on to say that "fields are still permitted to exceed 64-bits
so long as they aren't one contiguous string of bits". This patch breaks
that by always using a "uint64_t tmp" in the generated decodeToMCInst,
but it should be easy to fix in FilterChooser::emitBinaryParser by
choosing to use a different type of tmp based on the known total field
width.

I didn't try to implement the fix because I don't have a target that needs it so I wouldn't be able to test it. @dsanders do you?

[foad]

Ping! Does anyone have an interest in this for their own out-of-tree target?

[dsanders]

I didn't try to implement the fix because I don't have a target that needs it so I wouldn't be able to test it. @dsanders do you?

Sorry, I've gone to try and confirm this a couple times in the last week and things kept coming up. I'll take another look a moment

[dsanders]

In D98046#2629692, @dsanders wrote:

I didn't try to implement the fix because I don't have a target that needs it so I wouldn't be able to test it. @dsanders do you?

Sorry, I've gone to try and confirm this a couple times in the last week and things kept coming up. I'll take another look a moment

We've got a few that reach 64 but AFAICT none that exceed it

[dsanders]

LGTM

llvm/utils/TableGen/FixedLenDecoderEmitter.cpp

1125–1133

It looks like insertBits has implementations that cover both values of UseInsertBits. Would it make sense to always use insertBits here and rely on the type of tmp to pick the appropriate version of insertBits()?

[foad]

llvm/utils/TableGen/FixedLenDecoderEmitter.cpp

1125–1133

Using insertBits relies on us having written the initial tmp = 0; line, which we prefer to avoid for the simple cases. I think that's the only reason not to use insertBits in all cases.

[dsanders]

llvm/utils/TableGen/FixedLenDecoderEmitter.cpp

1125–1133

That makes sense to me. Thanks

llvm/test/TableGen/BitOffsetDecoder.td

def baz : Instruction {
     let AsmString = "baz  $factor";
     field bits<16> SoftFail = 0;
     }
 
 }
 
 // CHECK: tmp = fieldFromInstruction(insn, 8, 7);
 // CHECK: tmp = fieldFromInstruction(insn, 8, 8) << 3;
-// CHECK: tmp |= fieldFromInstruction(insn, 8, 4) << 7;
-// CHECK: tmp |= fieldFromInstruction(insn, 12, 4) << 3;
+// CHECK: insertBits(tmp, fieldFromInstruction(insn, 8, 4), 7, 4);
+// CHECK: insertBits(tmp, fieldFromInstruction(insn, 12, 4), 3, 4);
 // CHECK: tmp = fieldFromInstruction(insn, 8, 8) << 4;

llvm/test/TableGen/FixedLenDecoderEmitter/InitValue.td

def bax : Instruction {
     let factor{32} = 1; // non-zero initial value
     let Inst{15...8} = factor{32...25};
     }
 
 }
 
 // CHECK: tmp = fieldFromInstruction(insn, 9, 7) << 1;
 // CHECK: tmp = 0x1;
-// CHECK: tmp |= fieldFromInstruction(insn, 9, 7) << 1;
+// CHECK: insertBits(tmp, fieldFromInstruction(insn, 9, 7), 1, 7);
 // CHECK: tmp = 0x100000000;
-// CHECK: tmp |= fieldFromInstruction(insn, 8, 7) << 25;
+// CHECK: insertBits(tmp, fieldFromInstruction(insn, 8, 7), 25, 7);

llvm/utils/TableGen/FixedLenDecoderEmitter.cpp

emitDecoderFunction(formatted_raw_ostream &OS, DecoderSet &Decoders,
   // input decoder index.
   OS.indent(Indentation) << "template <typename InsnType>\n";
   OS.indent(Indentation) << "static DecodeStatus decodeToMCInst(DecodeStatus S,"
     << " unsigned Idx, InsnType insn, MCInst &MI,\n";
   OS.indent(Indentation) << "                                   uint64_t "
     << "Address, const void *Decoder, bool &DecodeComplete) {\n";
   Indentation += 2;
   OS.indent(Indentation) << "DecodeComplete = true;\n";
-  OS.indent(Indentation) << "InsnType tmp;\n";
+  // TODO: When InsnType is large, using uint64_t limits all fields to 64 bits
+  // It would be better for emitBinaryParser to use a 64-bit tmp whenever
+  // possible but fall back to an InsnType-sized tmp for truly large fields.
+  OS.indent(Indentation) << "using TmpType = "
+                            "std::conditional_t<std::is_integral<InsnType>::"
+                            "value, InsnType, uint64_t>;\n";
+  OS.indent(Indentation) << "TmpType tmp;\n";
   OS.indent(Indentation) << "switch (Idx) {\n";
   OS.indent(Indentation) << "default: llvm_unreachable(\"Invalid index!\");\n";
   unsigned Index = 0;
   for (const auto &Decoder : Decoders) {
     OS.indent(Indentation) << "case " << Index++ << ":\n";
     OS << Decoder;
     OS.indent(Indentation+2) << "return S;\n";
   }
unsigned FilterChooser::getIslands(std::vector<unsigned> &StartBits,
   return Num;
 }
 
 void FilterChooser::emitBinaryParser(raw_ostream &o, unsigned &Indentation,
                                      const OperandInfo &OpInfo,
                                      bool &OpHasCompleteDecoder) const {
   const std::string &Decoder = OpInfo.Decoder;
 
-  if (OpInfo.numFields() != 1 || OpInfo.InitValue != 0) {
+  bool UseInsertBits = OpInfo.numFields() != 1 || OpInfo.InitValue != 0;
+
+  if (UseInsertBits) {
     o.indent(Indentation) << "tmp = 0x";
     o.write_hex(OpInfo.InitValue);
     o << ";\n";
   }
 
   for (const EncodingField &EF : OpInfo) {
-    o.indent(Indentation) << "tmp ";
-    if (OpInfo.numFields() != 1 || OpInfo.InitValue != 0) o << '|';
-    o << "= fieldFromInstruction"
-      << "(insn, " << EF.Base << ", " << EF.Width << ')';
-    if (OpInfo.numFields() != 1 || EF.Offset != 0)
+    o.indent(Indentation);
+    if (UseInsertBits)
+      o << "insertBits(tmp, ";
+    else
+      o << "tmp = ";
+    o << "fieldFromInstruction(insn, " << EF.Base << ", " << EF.Width << ')';
+    if (UseInsertBits)
+      o << ", " << EF.Offset << ", " << EF.Width << ')';
+    else if (EF.Offset != 0)
       o << " << " << EF.Offset;
     o << ";\n";
   }
 
   if (Decoder != "") {
     OpHasCompleteDecoder = OpInfo.HasCompleteDecoder;
     o.indent(Indentation) << Emitter->GuardPrefix << Decoder
       << "(MI, tmp, Address, Decoder)"

 // fieldFromInstruction().
 // On Windows we make sure that this function is not inlined when
 // using the VS compiler. It has a bug which causes the function
 // to be optimized out in some circustances. See llvm.org/pr38292
 static void emitFieldFromInstruction(formatted_raw_ostream &OS) {
   OS << "// Helper functions for extracting fields from encoded instructions.\n"
      << "// InsnType must either be integral or an APInt-like object that "
         "must:\n"
-     << "// * Have a static const max_size_in_bits equal to the number of bits "
-        "in the\n"
-     << "//   encoding.\n"
      << "// * be default-constructible and copy-constructible\n"
      << "// * be constructible from a uint64_t\n"
      << "// * be constructible from an APInt (this can be private)\n"
-     << "// * Support getBitsSet(loBit, hiBit)\n"
-     << "// * be convertible to uint64_t\n"
-     << "// * Support the ~, &, ==, !=, and |= operators with other objects of "
+     << "// * Support insertBits(bits, startBit, numBits)\n"
+     << "// * Support extractBitsAsZExtValue(numBits, startBit)\n"
+     << "// * be convertible to bool\n"
+     << "// * Support the ~, &, ==, and != operators with other objects of "
         "the same type\n"
-     << "// * Support shift (<<, >>) with signed and unsigned integers on the "
-        "RHS\n"
      << "// * Support put (<<) to raw_ostream&\n"
      << "template <typename InsnType>\n"
      << "#if defined(_MSC_VER) && !defined(__clang__)\n"
      << "__declspec(noinline)\n"
      << "#endif\n"
-     << "static InsnType fieldFromInstruction(InsnType insn, unsigned "
-        "startBit,\n"
-     << "                                     unsigned numBits, "
-        "std::true_type) {\n"
+     << "static std::enable_if_t<std::is_integral<InsnType>::value, InsnType>\n"
+     << "fieldFromInstruction(const InsnType &insn, unsigned startBit,\n"
+     << "                     unsigned numBits) {\n"
      << "  assert(startBit + numBits <= 64 && \"Cannot support >64-bit "
         "extractions!\");\n"
      << "  assert(startBit + numBits <= (sizeof(InsnType) * 8) &&\n"
      << "         \"Instruction field out of bounds!\");\n"
      << "  InsnType fieldMask;\n"
      << "  if (numBits == sizeof(InsnType) * 8)\n"
      << "    fieldMask = (InsnType)(-1LL);\n"
      << "  else\n"
      << "    fieldMask = (((InsnType)1 << numBits) - 1) << startBit;\n"
      << "  return (insn & fieldMask) >> startBit;\n"
      << "}\n"
      << "\n"
      << "template <typename InsnType>\n"
-     << "static InsnType fieldFromInstruction(InsnType insn, unsigned "
-        "startBit,\n"
-     << "                                     unsigned numBits, "
-        "std::false_type) {\n"
-     << "  assert(startBit + numBits <= InsnType::max_size_in_bits && "
-        "\"Instruction field out of bounds!\");\n"
-     << "  InsnType fieldMask = InsnType::getBitsSet(0, numBits);\n"
-     << "  return (insn >> startBit) & fieldMask;\n"
+     << "static std::enable_if_t<!std::is_integral<InsnType>::value, "
+        "uint64_t>\n"
+     << "fieldFromInstruction(const InsnType &insn, unsigned startBit,\n"
+     << "                     unsigned numBits) {\n"
+     << "  return insn.extractBitsAsZExtValue(numBits, startBit);\n"
+     << "}\n\n";
+}
+
+// emitInsertBits - Emit the templated helper function insertBits().
+static void emitInsertBits(formatted_raw_ostream &OS) {
+  OS << "// Helper function for inserting bits extracted from an encoded "
+        "instruction into\n"
+     << "// a field.\n"
+     << "template <typename InsnType>\n"
+     << "static std::enable_if_t<std::is_integral<InsnType>::value>\n"
+     << "insertBits(InsnType &field, InsnType bits, unsigned startBit, "
+        "unsigned numBits) {\n"
+     << "  assert(startBit + numBits <= sizeof field * 8);\n"
+     << "  field |= (InsnType)bits << startBit;\n"
      << "}\n"
      << "\n"
      << "template <typename InsnType>\n"
-     << "static InsnType fieldFromInstruction(InsnType insn, unsigned "
-        "startBit,\n"
-     << "                                     unsigned numBits) {\n"
-     << "  return fieldFromInstruction(insn, startBit, numBits, "
-        "std::is_integral<InsnType>());\n"
+     << "static std::enable_if_t<!std::is_integral<InsnType>::value>\n"
+     << "insertBits(InsnType &field, uint64_t bits, unsigned startBit, "
+        "unsigned numBits) {\n"
+     << "  field.insertBits(bits, startBit, numBits);\n"
      << "}\n\n";
 }
 
 // emitDecodeInstruction - Emit the templated helper function
 // decodeInstruction().
 static void emitDecodeInstruction(formatted_raw_ostream &OS) {
   OS << "template <typename InsnType>\n"
      << "static DecodeStatus decodeInstruction(const uint8_t DecodeTable[], "
void FixedLenDecoderEmitter::run(raw_ostream &o) {
   OS << "#include \"llvm/Support/Debug.h\"\n";
   OS << "#include \"llvm/Support/LEB128.h\"\n";
   OS << "#include \"llvm/Support/raw_ostream.h\"\n";
   OS << "#include <assert.h>\n";
   OS << '\n';
   OS << "namespace llvm {\n\n";
 
   emitFieldFromInstruction(OS);
+  emitInsertBits(OS);
 
   Target.reverseBitsForLittleEndianEncoding();
 
   // Parameterize the decoders based on namespace and instruction width.
   std::set<StringRef> HwModeNames;
   const auto &NumberedInstructions = Target.getInstructionsByEnumValue();
   NumberedEncodings.reserve(NumberedInstructions.size());
   DenseMap<Record *, unsigned> IndexOfInstruction;