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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
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:
> > 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)`).
> C++ is hard.
Hi, this is causing a slight problem for us downstream because we're using a custom InsnType (not APInt). Have you seen the big comment emitted at line 2255? It explicitly says that your InsnType needs to "be constructible from a uint64_t". So please either update the comment, or stop using plain APInt as your InsnType :)


================
Comment at: llvm/utils/TableGen/FixedLenDecoderEmitter.cpp:2504
+     << "      bool Fail = (insn & PositiveMask) != 0 || (~insn & "
+        "NegativeMask) != 0;\n"
      << "      if (Fail)\n"
----------------
0x59616e wrote:
> myhsu wrote:
> > "!= 0" in these two lines are redundant.
> g++ screams "no match for ‘operator||’ (operand types are ‘llvm::APInt’ and ‘llvm::APInt’)"
Downstream the compiler complained about using mismatched types for operator&, InsnType and uint64_t. So please either change this back or at least update the comment on line 2255.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120958