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

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

myhsu added a comment.

In D120958#3382237 <https://reviews.llvm.org/D120958#3382237>, @0x59616e wrote:

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

This approach sounds reasonable to me



================
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) {
----------------
0x59616e wrote:
> 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.
The use of `getValueAsBitsInit` might be wrong but it's irrelevant to this patch. I think it's better to put this change into another patch.


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

https://reviews.llvm.org/D120958