[PATCH] D142218: [MC] Store implicit ops immediately after the TargetInsts table. NFC.

Sergei Barannikov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Jan 23 13:49:26 PST 2023


barannikov88 accepted this revision.
barannikov88 added a comment.
This revision is now accepted and ready to land.

LGTM with a couple of nits.
This change is significant, I think you need more votes to proceed.



================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:910
+  OS << "struct " << TargetName << "InstrTable {\n";
+  OS << "  MCInstrDesc Insts[" << NumberedInstructions.size() << "];\n";
+  OS << "  static_assert(alignof(MCInstrDesc) >= alignof(MCPhysReg), "
----------------
Could `Descs` be a better name? So that you don't access it as `Insts.Insts` below.



================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:949
+    OS << "    /* " << EmittedLists[List] << " */";
+    for (auto &Reg : List)
+      OS << ' ' << getQualifiedName(Reg) << ',';
----------------
Please make sure `auto` is qualified correctly if you prefer using it.
This `auto &` is really `Record *&`, while it should be `const Record *` (const non-reference).
Or just don't use it at all :) (as the coding style suggests).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142218



More information about the llvm-commits mailing list