[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#3416426 <https://reviews.llvm.org/D120958#3416426>, @0x59616e wrote:

> 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` ?

Sounds good to me, but I would suggest you to send a RFC to the forum asking for consensus.
Especially to see if there is any comment from existing FixedLenDecoder users (e.g. AArch64 and ARM). Also, in the RFC, please briefly explain why you want to build this feature on top of FixedLenDecoder rather than writing a separate disassembler.



================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2386
+     << "      unsigned PtrLen = 0;\n"
+     << "      uint64_t ExpectedValue = decodeULEB128(++Ptr, &PtrLen);\n"
+     << "      Ptr += PtrLen;\n"
----------------
why did you use another PtrLen variable here?


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:224
+  const RecordVal *RV = def.getValue(str);
+  if (BitsInit *bits = dyn_cast<BitsInit>(RV->getValue()))
+    return *bits;
----------------
0x59616e wrote:
> 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 ?
> 
> 
it's likely that those code were committed before LLVM coding style was solidified


================
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)
----------------
0x59616e wrote:
> 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 ?
> I've tried, and it remains the same.

that's fine, just a minor issue

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

I don't think so


================
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"
----------------
0x59616e wrote:
> 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"
> 
you're right and I was wrong about `APInt::operator=(uint64_t)`: the operator won't be used in the case of initialization (constructor will be used instead but APInt doesn't have `APInt(uint64_t)`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120958