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

[Martien de Jong via Phabricator via llvm-commits]

martien-de-jong 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
----------------
foad wrote:
> martien-de-jong wrote:
> > Could we give OpInfo the same treatment?
> Yes! D142219!
Ah, many small steps make a great leap.


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:922
+  OS << "#undef GET_INSTRINFO_MC_DESC\n";
+  OS << "namespace llvm {\n\n";
 
----------------
foad wrote:
> 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!
I'll start digging and removing the rust.


================
Comment at: llvm/utils/TableGen/InstrInfoEmitter.cpp:1070
+  OS << "extern const " << TargetName << "InstrTable " << TargetName
+     << "Descs;\n";
   OS << "extern const unsigned " << TargetName << "InstrNameIndices[];\n";
----------------
foad wrote:
> 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.
Right. I don't grasp the details right away, but I think it is a perfect example of methods that could be added to the interface class, or in a local class derived from it.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142218