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

[Sheng via Phabricator via llvm-commits]

0x59616e added a comment.

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

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

What about `DecoderEmitter.cpp` ?



================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:224
+  const RecordVal *RV = def.getValue(str);
+  if (BitsInit *bits = dyn_cast<BitsInit>(RV->getValue()))
+    return *bits;
----------------
myhsu wrote:
> local variables should start with uppercase 
Just wondering: there are some variables that is started with lowercase in the original code. For example the parameter "&def" in this function.

Is that a mistake ? or is there any special naming rule that I don't know ?




================
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)
----------------
myhsu wrote:
> Did you use `clang-format-diff.py` rather than clang-format? I wonder why this line changed...
I've tried, and it remains the same.

BTW I use "git clang-format". is there any difference between these two ?


================
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"
----------------
myhsu wrote:
> why change to uint64_t? If you're worry about InsnType being APInt, APInt has `operator=(uint64_t)`
But it yells "conversion from ‘uint64_t’ {aka ‘long unsigned int’} to non-scalar type ‘llvm::APInt’ requested"



================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2504
+     << "      bool Fail = (insn & PositiveMask) != 0 || (~insn & "
+        "NegativeMask) != 0;\n"
      << "      if (Fail)\n"
----------------
myhsu wrote:
> "!= 0" in these two lines are redundant.
g++ screams "no match for ‘operator||’ (operand types are ‘llvm::APInt’ and ‘llvm::APInt’)"


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120958