[PATCH] D115128: [TableGen][CodeEmitter] Introducing the VarLenCodeEmitterGen infrastructure

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

myhsu added inline comments.


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:235
+     << "    Msg << \"Not supported instr: \" << MI;\n"
+     << "    report_fatal_error(Msg.str().c_str());\n"
+     << "  }\n";
----------------
craig.topper wrote:
> Can this use a Twine to build the message as part of the argument to report_fatal_error?
Unfortunately MCInst (i.e. MI here) doesn't have something like toString so using Twine might make the code here less concise.


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:266
+  if (HwMode == -1)
+    OS << "  static const APInt InstBits[] = {\n";
+  else
----------------
craig.topper wrote:
> I think this violates the no static global constructor rule?
no, `InstBits` (and `InstBits_<HW mode>`) is a local variable. Here is how it looks in the generated .inc file:
```
void M68kMCCodeEmitter::getBinaryCodeForInstr(const MCInst &MI,
    SmallVectorImpl<MCFixup> &Fixups,
    APInt &Inst,
    APInt &Scratch,
    const MCSubtargetInfo &STI) const {
  static const APInt InstBits[] = {
    APInt::getZeroWidth(),
    APInt::getZeroWidth(),
    APInt::getZeroWidth(),
...
```


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:126
+      // Normalization: Hi bit should always be the second argument.
+      ArrayRef<Init *> NewArgs = {OperandName, LoBit, HiBit};
+      Segments.push_back(
----------------
craig.topper wrote:
> This code is dangerous. It will create a dangling reference to an initializer_list that goes out of scope when the line ends. Use a plain C array here.
I don't think it will create a dangling reference in this particular case because DagInit::get actually [[ https://github.com/llvm/llvm-project/blob/93fbaa46c82abd2d54bce9a7c3b39b01c30220d6/llvm/lib/TableGen/Record.cpp#L2120 | copies ]] NewArgs' elements. But I'm changing to plain C array anyway.


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

https://reviews.llvm.org/D115128