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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/include/llvm/MC/MCInstrDesc.h:214
   uint64_t TSFlags;              // Target Specific Flag values
-  const MCPhysReg *ImplicitOps;  // List of implicit uses followed by defs
   const MCOperandInfo *OpInfo;   // 'NumOperands' entries about operands
----------------
martien-de-jong wrote:
> Could we give OpInfo the same treatment?
Yes! D142219!


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:922
+  OS << "#undef GET_INSTRINFO_MC_DESC\n";
+  OS << "namespace llvm {\n\n";
 
----------------
martien-de-jong wrote:
> In some private table gen backend that also used these offset tables, we went away from the one raw out stream. Instead, there's an array abstraction that builds the body of the array in a stringstream and methods to generate the reference string (pointer to some element, or its index or offset) for e.g. the last added entry, or a given offset from a local base.  It allows you to build all tables in one intuitive sweep, and then emit the accumulated bodies in some order to the output stream.  If there's interest, I could probably share that table abstraction to add as common facility to tablegen.
Sounds nice!


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:1070
+  OS << "extern const " << TargetName << "InstrTable " << TargetName
+     << "Descs;\n";
   OS << "extern const unsigned " << TargetName << "InstrNameIndices[];\n";
----------------
martien-de-jong wrote:
> I'm dreaming of getting rid of these extern auxiliary tables.  Perhaps we can start by packing all of these addresses in a struct that can serve as an interface class? We can then make them static and only export that struct. In the future, that structure will then be the only thing that needs to be relocated.
> Hiding the current names would expose the current users.
Sounds reasonable. The only rogue user of TargetInsts/TargetDescs that I'm aware of is the ARM AsmParser that I had to update in this patch. There was some discussion about how to fix that in D142217 but I didn't have the ARM expertise to do it myself.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142218