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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:152
+    if (const RecordVal *RV = R->getValue("EncodingInfos")) {
+      if (DefInit *DI = dyn_cast_or_null<DefInit>(RV->getValue())) {
+        EncodingInfoByHwMode EBM(DI->getDef(), HWM);
----------------
You can use auto here.


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:235
+     << "    Msg << \"Not supported instr: \" << MI;\n"
+     << "    report_fatal_error(Msg.str().c_str());\n"
+     << "  }\n";
----------------
Can this use a Twine to build the message as part of the argument to report_fatal_error?


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:266
+  if (HwMode == -1)
+    OS << "  static const APInt InstBits[] = {\n";
+  else
----------------
I think this violates the no static global constructor rule?


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:371
+      unsigned LoBit = 0U;
+      if (const auto *SV = dyn_cast<StringInit>(Val))
+        OperandName = SV->getValue();
----------------
Curly braces to match the else


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:391
+                   << utostr(FlatOpIdx) << "), Scratch, Fixups, STI);\n";
+      std::string ExtractStr =
+          "Scratch.extractBits(" + utostr(NumBits) + ", " + utostr(LoBit) + ")";
----------------
Why is this a separate std::string and not part of the stream?


================
Comment at: llvm/utils/TableGen/VarLenCodeEmitterGen.cpp:403
+
+  return std::move(SS.str());
+}
----------------
Can this be `return Case` like the other changes in D115374


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

https://reviews.llvm.org/D115128