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

[Sheng via Phabricator via llvm-commits]

0x59616e added a comment.

In D120958#3380177 <https://reviews.llvm.org/D120958#3380177>, @myhsu wrote:

> 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.

Sorry for not having time implementing my ideas, so I just run through it quickly (I will do it maybe this Saturday or Sunday):

We can traverse the Segments of VarLenInst and use  `CGIOperandList::OperandInfo::ParseOperandName`  to get the operand number, and then add that Segments into the correspoding `OpInfo` according to the operand number.

For example, if we find "dst.reg" and use `CGIOperandList::OperandInfo::ParseOperandName`  and know "Oh, this is the third operand", then we can add this into the third `OpInfo`.

In this way we can avoid overload `BitsInit` completely.



================
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) {
----------------
myhsu wrote:
> why do you want to change these three lines? I don't think `RV` is used anywhere else.
My understanding is: `SFBits` could be null pointer (take a look at the if statement below), which means `SoftFail` is not necessary. But using `getValueAsBitsInit` will trigger assertion if `SoftFail` can not be found.


================
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;
+
----------------
myhsu wrote:
> ditto
Same reason as above: `SFBits` is not necessary (if not then the if statement below is useless) but `getValueAsBitsInit` dislike that.


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2303
+     << "                               = [](InsnType &insn, uint64_t){}) {\n"
+     << "  makeUp(insn, startBit + numBits);\n"
      << "  return insn.extractBitsAsZExtValue(numBits, startBit);\n"
----------------
myhsu wrote:
> 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?
In some cases, for example, case `MCD::OPC_CheckField`,  needs to extract the bit field before we know the length of the instruction.

But only two cases need this IIRC: `OPC_ExtractField` and `OPC_CheckField`. Doing this in `fieldFromInstruction` is because I'm lazy. I will fix this in the next diff.

And, can we resize `insn` ahead of time and do it only once ? I think it's no. We have to know the length to resize `insn`. So we can do this only in  case `MCD::OPC_Decode`.


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

https://reviews.llvm.org/D120958