[CodeEmitter] Support instruction widths > 64 bits

[jmolloy]

Summary

Some VLIW instruction sets are Very Long Indeed. Using uint64_t constricts the Inst encoding to 64 bits (naturally).

This change switches CodeEmitter to a mode that uses APInts when Inst's bitwidth is > 64 bits (NFC for existing targets).

When Inst.BitWidth > 64 the prototype changes to:

void TargetMCCodeEmitter::getBinaryCodeForInstr(const MCInst &MI,
                                                SmallVectorImpl<MCFixup> &Fixups,
                                                APInt &Inst,
                                                APInt &Scratch,
                                                const MCSubtargetInfo &STI);

The Inst parameter returns the encoded instruction, the Scratch parameter is used internally for manipulating operands and is exposed so that the underlying storage can be reused between calls to getBinaryCodeForInstr. The goal is to elide any APInt constructions that we can.

Similarly the operand encoding prototype changes to:

getMachineOpValue(const MCInst &MI, const MCOperand &MO, APInt &op, SmallVectorImpl<MCFixup> &Fixups, const MCSubtargetInfo &STI);

That is, the operand is passed by reference as APInt rather than returned as uint64_t.

To reiterate, this APInt mode is enabled only when Inst.BitWidth > 64, so while no existing in-tree target makes use of that,
there's basic test coverage in llvm/test/TableGen/RegisterEncoder.td.

[bkramer]

I think this is fine. Features like this might bitrot quickly but that's on you ;)

llvm/utils/TableGen/CodeEmitterGen.cpp

309

utostr() isn't needed when printing to a raw_ostream

[lebedev.ri]

Can you quote where https://llvm.org/docs/DeveloperPolicy.html#test-cases says it's okay to commit dead code with no test coverage?

[xbolva00]

There are tests in llvm/test/TableGen/BigEncoder.td.

[lebedev.ri]

On Fri, Sep 13, 2019 at 2:03 PM James Molloy <[e-mail]> wrote:

Hi Roman,

There is one diff to a test and one brand new test in this change (llvm/test/TableGen/BigEncoder.td).

Re test coverage: that is not really how the patch description was being read, usually that wording means something different.

As a sidenote, your message could be considered by some unnecessarily passive-aggressive.

Cheers,

James

[tra]

llvm/test/TableGen/BigEncoder.td

2

I'd add some details explaining what the test does and why.
Otherwise it's not clear what this test does and why without the full context of the patch.
Those bits<65> are not very noticeable on their own.

59–69

checks for results of bar/baz are missing.

llvm/test/TableGen/BigEncoder.td

+// RUN: llvm-tblgen -gen-emitter -I %p/../../include %s | FileCheck %s
+
+include "llvm/Target/Target.td"
+
+def archInstrInfo : InstrInfo { }
+
+def arch : Target {
+    let InstructionSet = archInstrInfo;
+}
+
+def  Myi32  : Operand<i32> {
+  let DecoderMethod = "DecodeMyi32";
+}
+
+
+let OutOperandList = (outs), Size = 2 in {
+
+def foo : Instruction {
+    let InOperandList = (ins i32imm:$factor);
+    field bits<65> Inst;
+    bits<32> factor;
+    let Inst{7-0} = 0xAA;
+    let Inst{14-8} = factor{6-0}; // no offset
+    let AsmString = "foo  $factor";
+    field bits<16> SoftFail = 0;
+    }
+
+def bar : Instruction {
+    let InOperandList = (ins i32imm:$factor);
+    field bits<65> Inst;
+    bits<32> factor;
+    let Inst{7-0} = 0xBB;
+    let Inst{15-8} = factor{10-3}; // offset by 3
+    let AsmString = "bar  $factor";
+    field bits<16> SoftFail = 0;
+    }
+
+def biz : Instruction {
+    let InOperandList = (ins i32imm:$factor);
+    field bits<65> Inst;
+    bits<32> factor;
+    let Inst{7-0} = 0xCC;
+    let Inst{11-8,15-12} = factor{10-3}; // offset by 3, multipart
+    let AsmString = "biz  $factor";
+    field bits<16> SoftFail = 0;
+    }
+
+def baz : Instruction {
+    let InOperandList = (ins Myi32:$factor);
+    field bits<65> Inst;
+    bits<32> factor;
+    let Inst{7-0} = 0xDD;
+    let Inst{15-8} = factor{11-4}; // offset by 4 + custom decode
+    let AsmString = "baz  $factor";
+    field bits<16> SoftFail = 0;
+    }
+
+}
+// CHECK-LABEL: case ::biz: {
+// CHECK: const APInt [[x:M[0-9]+]] = APInt::getBitsSet(65, 3, 7);
+// CHECK-NEXT: Value |= (op & [[x]]) << 9;
+// CHECK-NEXT: const APInt [[y:M[0-9]+]] = APInt::getBitsSet(65, 7, 11);
+// CHECK-NEXT: Value |= (op & [[y]]) << 1;
+
+// CHECK-LABEL: case ::foo: {
+// CHECK: const APInt [[x:M[0-9]+]] = APInt::getBitsSet(65, 0, 7);
+// CHECK-NEXT: op &= [[x]];
+// CHECK-NEXT: op <<= 8;
+// CHECK-NEXT: Value |= op;

llvm/test/TableGen/RegisterEncoder.td

def foo : Instruction {
   let InOperandList = (ins RegOperand:$bar);
 
   bits<8> bar;
   bits<8> Inst = bar;
 }
 
 // CHECK: case ::foo: {
 // CHECK:   op = barEncoder
-// CHECK:   Value |= op & UINT64_C(255);
+// CHECK:   op &= UINT64_C(255);
+// CHECK:   Value |= op;
 // CHECK:   break;
-// CHECK: }
+// CHECK: }
 

llvm/utils/TableGen/CodeEmitterGen.cpp

 // returns the (currently, 32-bit unsigned) value of the instruction.
 //
 //===----------------------------------------------------------------------===//
 
 #include "CodeGenInstruction.h"
 #include "CodeGenTarget.h"
 #include "SubtargetFeatureInfo.h"
 #include "Types.h"
+#include "llvm/ADT/APInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/TableGen/Record.h"
 #include "llvm/TableGen/TableGenBackend.h"
 #include <cassert>
 #include <cstdint>
private:
   int getVariableBit(const std::string &VarName, BitsInit *BI, int bit);
   std::string getInstructionCase(Record *R, CodeGenTarget &Target);
   void AddCodeToMergeInOperand(Record *R, BitsInit *BI,
                                const std::string &VarName,
                                unsigned &NumberedOp,
                                std::set<unsigned> &NamedOpIndices,
                                std::string &Case, CodeGenTarget &Target);
 
+  unsigned BitWidth;
+  bool UseAPInt;
 };
 
 // If the VarBitInit at position 'bit' matches the specified variable then
 // return the variable bit position.  Otherwise return -1.
 int CodeEmitterGen::getVariableBit(const std::string &VarName,
                                    BitsInit *BI, int bit) {
   if (VarBitInit *VBI = dyn_cast<VarBitInit>(BI->getBit(bit))) {
     if (VarInit *VI = dyn_cast<VarInit>(VBI->getBitVar()))
while (NumberedOp < NumberOps &&
       }
     }
 
     OpIdx = NumberedOp++;
   }
   
   std::pair<unsigned, unsigned> SO = CGI.Operands.getSubOperandNumber(OpIdx);
   std::string &EncoderMethodName = CGI.Operands[SO.first].EncoderMethodName;
-  
+  if (UseAPInt)
+    Case += "      op.clearAllBits();\n";
+
   // If the source operand has a custom encoder, use it. This will
   // get the encoding for all of the suboperands.
   if (!EncoderMethodName.empty()) {
     // A custom encoder has all of the information for the
     // sub-operands, if there are more than one, so only
     // query the encoder once per source operand.
     if (SO.second == 0) {
-      Case += "      // op: " + VarName + "\n" +
-              "      op = " + EncoderMethodName + "(MI, " + utostr(OpIdx);
-      Case += ", Fixups, STI";
-      Case += ");\n";
+      Case += "      // op: " + VarName + "\n";
+      if (UseAPInt) {
+        Case += "      " + EncoderMethodName + "(MI, " + utostr(OpIdx);
+        Case += ", op";
+      } else {
+        Case += "      op = " + EncoderMethodName + "(MI, " + utostr(OpIdx);
       }
+      Case += ", Fixups, STI);\n";
+    }
+  } else {
+    Case += "      // op: " + VarName + "\n";
+    if (UseAPInt) {
+      Case += "      getMachineOpValue(MI, MI.getOperand(" + utostr(OpIdx) + ")";
+      Case += ", op, Fixups, STI";
     } else {
-    Case += "      // op: " + VarName + "\n" +
-      "      op = getMachineOpValue(MI, MI.getOperand(" + utostr(OpIdx) + ")";
+      Case += "      op = getMachineOpValue(MI, MI.getOperand(" + utostr(OpIdx) + ")";
       Case += ", Fixups, STI";
+    }
     Case += ");\n";
   }
-  
+  // Precalculate the number of lits this variable contributes to in the
+  // operand. If there is a single lit (consecutive range of bits) we can use a
+  // destructive sequence on APInt that reduces memory allocations.
+  int numOperandLits = 0;
+  for (int tmpBit = bit; tmpBit >= 0;) {
+    int varBit = getVariableBit(VarName, BI, tmpBit);
+
+    // If this bit isn't from a variable, skip it.
+    if (varBit == -1) {
+      --tmpBit;
+      continue;
+    }
+
+    // Figure out the consecutive range of bits covered by this operand, in
+    // order to generate better encoding code.
+    int beginVarBit = varBit;
+    int N = 1;
+    for (--tmpBit; tmpBit >= 0;) {
+      varBit = getVariableBit(VarName, BI, tmpBit);
+      if (varBit == -1 || varBit != (beginVarBit - N))
+        break;
+      ++N;
+      --tmpBit;
+    }
+    ++numOperandLits;
+  }
+
   for (; bit >= 0; ) {
     int varBit = getVariableBit(VarName, BI, bit);
     
     // If this bit isn't from a variable, skip it.
     if (varBit == -1) {
       --bit;
       continue;
     }
     
     // Figure out the consecutive range of bits covered by this operand, in
     // order to generate better encoding code.
     int beginInstBit = bit;
     int beginVarBit = varBit;
     int N = 1;
     for (--bit; bit >= 0;) {
       varBit = getVariableBit(VarName, BI, bit);
       if (varBit == -1 || varBit != (beginVarBit - N)) break;
       ++N;
       --bit;
     }
-     
-    uint64_t opMask = ~(uint64_t)0 >> (64-N);
-    int opShift = beginVarBit - N + 1;
+    std::string maskStr;
+    int opShift;
+
+    if (UseAPInt) {
+      unsigned loBit = beginVarBit - N + 1;
+      unsigned hiBit = loBit + N;
+      maskStr = "M" + itostr(bit);
+      Case += "      const APInt " + maskStr + " = APInt::getBitsSet(" +
+              itostr(BitWidth) + ", " + itostr(loBit) + ", " + itostr(hiBit) +
+              ");\n";
+    } else {
+      uint64_t opMask = ~(uint64_t)0 >> (64 - N);
+      opShift = beginVarBit - N + 1;
       opMask <<= opShift;
+      maskStr = "UINT64_C(" + utostr(opMask) + ")";
+    }
     opShift = beginInstBit - beginVarBit;
-    
+    if (numOperandLits == 1) {
+      // Because Op may be an APInt, ensure all arithmetic is done in-place
+      // where possible to elide copies.
+      Case += "      op &= " + maskStr + ";\n";
       if (opShift > 0) {
-      Case += "      Value |= (op & UINT64_C(" + utostr(opMask) + ")) << " +
-              itostr(opShift) + ";\n";
+        Case += "      op <<= " + itostr(opShift) + ";\n";
       } else if (opShift < 0) {
-      Case += "      Value |= (op & UINT64_C(" + utostr(opMask) + ")) >> " + 
-              itostr(-opShift) + ";\n";
+        Case += "      op >>= " + itostr(-opShift) + ";\n";
+      }
+      Case += "      Value |= op;\n";
     } else {
-      Case += "      Value |= op & UINT64_C(" + utostr(opMask) + ");\n";
+      if (opShift > 0) {
+        Case += "      Value |= (op & " + maskStr + ") << " + itostr(opShift) +
+                ";\n";
+      } else if (opShift < 0) {
+        Case += "      Value |= (op & " + maskStr + ") >> " + itostr(-opShift) +
+                ";\n";
+      } else {
+        Case += "      Value |= (op & " + maskStr + ");\n";
+      }
     }
   }
 }
 
 std::string CodeEmitterGen::getInstructionCase(Record *R,
                                                CodeGenTarget &Target) {
   std::string Case;
   BitsInit *BI = R->getValueAsBitsInit("Inst");

 static std::string
 getNameForFeatureBitset(const std::vector<Record *> &FeatureBitset) {
   std::string Name = "CEFBS";
   for (const auto &Feature : FeatureBitset)
     Name += ("_" + Feature->getName()).str();
   return Name;
 }
 
+static void emitInstBits(raw_ostream &OS, const APInt &Bits) {
+  for (unsigned I = 0; I < Bits.getNumWords(); ++I)
+    OS << ((I > 0) ? ", " : "") << "UINT64_C(" << utostr(Bits.getRawData()[I])
+       << ")";
+}
+
 void CodeEmitterGen::run(raw_ostream &o) {
   CodeGenTarget Target(Records);
   std::vector<Record*> Insts = Records.getAllDerivedDefinitions("Instruction");
 
   // For little-endian instruction bit encodings, reverse the bit order
   Target.reverseBitsForLittleEndianEncoding();
 
   ArrayRef<const CodeGenInstruction*> NumberedInstructions =
     Target.getInstructionsByEnumValue();
 
+  // Default to something sensible in case the target doesn't define Inst.
+  BitWidth = 32;
+  for (const CodeGenInstruction *CGI : NumberedInstructions) {
+    Record *R = CGI->TheDef;
+    if (R->getValueAsString("Namespace") == "TargetOpcode" ||
+        R->getValueAsBit("isPseudo"))
+      continue;
+
+    BitsInit *BI = R->getValueAsBitsInit("Inst");
+    BitWidth = BI->getNumBits();
+    break;
+  }
+  UseAPInt = BitWidth > 64;
+  
   // Emit function declaration
+  if (UseAPInt) {
+    o << "void " << Target.getName()
+      << "MCCodeEmitter::getBinaryCodeForInstr(const MCInst &MI,\n"
+      << "    SmallVectorImpl<MCFixup> &Fixups,\n"
+      << "    APInt &Inst,\n"
+      << "    APInt &Scratch,\n"
+      << "    const MCSubtargetInfo &STI) const {\n";
+  } else {
     o << "uint64_t " << Target.getName();
     o << "MCCodeEmitter::getBinaryCodeForInstr(const MCInst &MI,\n"
       << "    SmallVectorImpl<MCFixup> &Fixups,\n"
       << "    const MCSubtargetInfo &STI) const {\n";
+  }
   
   // Emit instruction base values
   o << "  static const uint64_t InstBits[] = {\n";
   for (const CodeGenInstruction *CGI : NumberedInstructions) {
     Record *R = CGI->TheDef;
 
     if (R->getValueAsString("Namespace") == "TargetOpcode" ||
         R->getValueAsBit("isPseudo")) {
-      o << "    UINT64_C(0),\n";
+      o << "    "; emitInstBits(o, APInt(BitWidth, 0)); o << ",\n";
       continue;
     }
 
     BitsInit *BI = R->getValueAsBitsInit("Inst");
+    BitWidth = BI->getNumBits();
 
     // Start by filling in fixed values.
-    uint64_t Value = 0;
+    APInt Value(BitWidth, 0);
     for (unsigned i = 0, e = BI->getNumBits(); i != e; ++i) {
-      if (BitInit *B = dyn_cast<BitInit>(BI->getBit(e-i-1)))
-        Value |= (uint64_t)B->getValue() << (e-i-1);
+      if (BitInit *B = dyn_cast<BitInit>(BI->getBit(e - i - 1)))
+        Value |= APInt(BitWidth, (uint64_t)B->getValue()) << (e - i - 1);
     }
-    o << "    UINT64_C(" << Value << ")," << '\t' << "// " << R->getName() << "\n";
+    o << "    ";
+    emitInstBits(o, Value);
+    o << "," << '\t' << "// " << R->getName() << "\n";
   }
   o << "    UINT64_C(0)\n  };\n";
   
   // Map to accumulate all the cases.
   std::map<std::string, std::vector<std::string>> CaseMap;
 
   // Construct all cases statement for each opcode
   for (std::vector<Record*>::iterator IC = Insts.begin(), EC = Insts.end();
         IC != EC; ++IC) {
     Record *R = *IC;
     if (R->getValueAsString("Namespace") == "TargetOpcode" ||
         R->getValueAsBit("isPseudo"))
       continue;
     std::string InstName =
         (R->getValueAsString("Namespace") + "::" + R->getName()).str();
     std::string Case = getInstructionCase(R, Target);
 
     CaseMap[Case].push_back(std::move(InstName));
   }
 
   // Emit initial function code
+  if (UseAPInt) {
+    int NumWords = APInt::getNumWords(BitWidth);
+    o << "  const unsigned opcode = MI.getOpcode();\n"
+      << "  if (Inst.getBitWidth() != " << BitWidth << ")\n"
+      << "    Inst = Inst.zext(" << BitWidth << ");\n"
+      << "  if (Scratch.getBitWidth() != " << BitWidth << ")\n"
+      << "    Scratch = Scratch.zext(" << BitWidth << ");\n"
+      << "  std::copy(&InstBits[opcode * " << NumWords << "],\n"
+      << "            &InstBits[(opcode + 1) * " << NumWords << "],\n"
+      << "            Inst.getRawData());\n"
+      << "  APInt &op = Scratch;\n"
+      << "  switch (opcode) {\n";
+  } else {
     o << "  const unsigned opcode = MI.getOpcode();\n"
       << "  uint64_t Value = InstBits[opcode];\n"
       << "  uint64_t op = 0;\n"
       << "  (void)op;  // suppress warning\n"
       << "  switch (opcode) {\n";
+  }
 
   // Emit each case statement
   std::map<std::string, std::vector<std::string>>::iterator IE, EE;
   for (IE = CaseMap.begin(), EE = CaseMap.end(); IE != EE; ++IE) {
     const std::string &Case = IE->first;
     std::vector<std::string> &InstList = IE->second;
 
     for (int i = 0, N = InstList.size(); i < N; i++) {