[PATCH] D120958: [TableGen] Add support for variable length instruction in decoder generator

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu added a comment.

Thank you for the patch.
A high level question: It seems like generating a decoder requires two phases -- First, generates a list of `OperandInfo`, which is currently done by `populateInstruction`, then emit the real (C++) code according to these `OperandInfo` instances. My question is, can we use our own way -- possibly putting in a separate file -- to generate these `OperandInfo` before supplying them to the second phase, instead of trying to piggyback everything into the existing FixedLenDecoder framework?
Because, to be honest, I'm not a big fan of overloading the `BitsInit` (to carry operand info). Using a `BitsInit` might fit well for fixed-length instructions but I feel like there are more elegant ways to handle var-length instructions. For instance, using `CGIOperandList::OperandInfo::ParseOperandName` to parse suboperands rather than traversing every single in/out operands in the original instruction definition.

Also, please double check all your modifications and make sure to follow the LLVM coding style, especially the naming convention of local variables.



================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:228
+  // variable length instruction
+  DagInit *dag = cast<DagInit>(RV->getValue());
+  VarLenInst VLI = VarLenInst(dag, RV);
----------------
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:265
+    } else {
+      llvm_unreachable("What kind of init it is ?");
+    }
----------------
"unknown Init kind" would probably be better.


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:498
+        AllInstructions[Opcode].EncodingDef->getValue("SoftFail");
+    const BitsInit *SFBits = RV ? dyn_cast<BitsInit>(RV->getValue()) : nullptr;
+    for (unsigned i = 0; i < Bits.getNumBits(); ++i) {
----------------
why do you want to change these three lines? I don't think `RV` is used anywhere else.


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:1388
+  const RecordVal *RV = AllInstructions[Opc].EncodingDef->getValue("SoftFail");
+  BitsInit *SFBits = RV ? dyn_cast<BitsInit>(RV->getValue()) : nullptr;
+
----------------
ditto


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2149
 
-    unsigned Base = ~0U;
-    unsigned Width = 0;
-    unsigned Offset = 0;
+    auto getOpInfo = [&](OperandInfo OI, StringRef SubOpName) {
+      unsigned Base = ~0U;
----------------
The `OI` argument can be `OperandInfo &` to avoid copy.


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2300
+     << "                     unsigned numBits,\n"
+     << "                     std::function<void(InsnType &, uint64_t)> "
+        "makeUp\n"
----------------
please use `llvm::function_ref` here


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2303
+     << "                               = [](InsnType &insn, uint64_t){}) {\n"
+     << "  makeUp(insn, startBit + numBits);\n"
      << "  return insn.extractBitsAsZExtValue(numBits, startBit);\n"
----------------
why do we want to enlarge `insn` (on-demand) upon every field extractions? Can we resize `insn` ahead of time and do it only once?


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2342
+    OS << ",\n"
+       << "                                      std::function<void(APInt &,"
+       << " uint64_t)> makeUp";
----------------
ditto


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

https://reviews.llvm.org/D120958