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

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

myhsu added a comment.

The logics look cleaner now.
Now a bigger question is: Should we still calling it //FixedLen//DecoderEmitter?



================
Comment at: llvm/test/TableGen/VarLenEncoder.td:3
+// RUN: llvm-tblgen -gen-disassembler -I %p/../../include %s | \
+// RUN:     FileCheck --check-prefix=CHECK-DECODE %s
 
----------------
I still prefer the decoder's tests either go under the FixedLenDecoderEmitter's folder or at least putting into a separate file.


================
Comment at: llvm/test/TableGen/VarLenEncoder.td:51
+        !isa<MemOp32>(memory_op) : 0b1,
+      )
+    ),
----------------
I don't quite understand what this `!cond` trying to do


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:224
+  const RecordVal *RV = def.getValue(str);
+  if (BitsInit *bits = dyn_cast<BitsInit>(RV->getValue()))
+    return *bits;
----------------
local variables should start with uppercase 


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:1868
+  VarLenInst VLI(cast<DagInit>(RV->getValue()), RV);
+  SmallVector<int, 3> TiedTo;
+
----------------
I'm not sure non-power-of-two # of (inlined) elements for SmallVector is recommended. If you're not sure what number to put, just use SmallVector<int> as recommended [[ https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h | here ]].


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:1875
+        Operands.push_back(getOpInfo(cast<DefInit>(Arg)->getDef()));
+      }
+    } else {
----------------
braces here can be removed


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:1900
+
+    if (OpName != "") {
+      auto OpSubOpPair =
----------------
please use `!OpName.empty()` or `OpName.size()`


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:1958
+    InOutOperands.push_back(
+        std::make_pair(Out->getArg(i), Out->getArgNameStr(i)));
   for (unsigned i = 0; i < In->getNumArgs(); ++i)
----------------
Did you use `clang-format-diff.py` rather than clang-format? I wonder why this line changed...


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2362
      << "      unsigned Len;\n"
-     << "      InsnType Val = decodeULEB128(++Ptr, &Len);\n"
+     << "      uint64_t Val = decodeULEB128(++Ptr, &Len);\n"
      << "      Ptr += Len;\n"
----------------
why change to uint64_t? If you're worry about InsnType being APInt, APInt has `operator=(uint64_t)`


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2389
+     << "      unsigned PtrLen = 0;\n"
+     << "      uint64_t ExpectedValue = decodeULEB128(++Ptr, &PtrLen);\n"
+     << "      Ptr += PtrLen;\n"
----------------
ditto


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2499
      << "      unsigned Len;\n"
-     << "      InsnType PositiveMask = decodeULEB128(++Ptr, &Len);\n"
+     << "      uint64_t PositiveMask = decodeULEB128(++Ptr, &Len);\n"
      << "      Ptr += Len;\n"
----------------
ditto


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2504
+     << "      bool Fail = (insn & PositiveMask) != 0 || (~insn & "
+        "NegativeMask) != 0;\n"
      << "      if (Fail)\n"
----------------
"!= 0" in these two lines are redundant.


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2543
   std::set<StringRef> HwModeNames;
-  const auto &NumberedInstructions = Target.getInstructionsByEnumValue();
+  const auto NumberedInstructions = Target.getInstructionsByEnumValue();
   NumberedEncodings.reserve(NumberedInstructions.size());
----------------
why remove '&'?


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2677
+  if (IsVarLenInst)
+    emitInstrLenTable(OS, InstrLen);
   // Emit the predicate function.
----------------
Could we add some comments here?


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.h:63
+
 class RecordKeeper;
 class raw_ostream;
----------------
I think these forward declarations are obsolete now


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120958