[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