This shrinks MCInstrDesc (and hence the whole TargetInsts table) because
we can store a 16-bit offset value to access the implicit operands,
instead of a pointer. This also reduces the number of relocs that need
to be applied when LLVM is compiled as position-independent code.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/MC/MCInstrDesc.h | ||
---|---|---|
579–580 | This looks a bit undefined to me. Is ubsan and asan happy with this? |
llvm/include/llvm/MC/MCInstrDesc.h | ||
---|---|---|
579–580 | Yes. I ran check-llvm with LLVM_USE_SANITIZER='Address;Undefined' and it is clean. |
LGTM with a couple of nits.
This change is significant, I think you need more votes to proceed.
llvm/utils/TableGen/InstrInfoEmitter.cpp | ||
---|---|---|
910 | Could Descs be a better name? So that you don't access it as Insts.Insts below. | |
949 | Please make sure auto is qualified correctly if you prefer using it. |
llvm/utils/TableGen/InstrInfoEmitter.cpp | ||
---|---|---|
910 | Maybe. It's actually e.g. AMDGPUInsts.Insts, but I guess your point still applies. |
llvm/include/llvm/MC/MCInstrDesc.h | ||
---|---|---|
214 | Could we give OpInfo the same treatment? | |
llvm/utils/TableGen/InstrInfoEmitter.cpp | ||
1070 | 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. |
llvm/utils/TableGen/InstrInfoEmitter.cpp | ||
---|---|---|
922 | 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. |
llvm/include/llvm/MC/MCInstrDesc.h | ||
---|---|---|
214 | Yes! D142219! | |
llvm/utils/TableGen/InstrInfoEmitter.cpp | ||
922 | Sounds nice! | |
1070 | 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. |
llvm/include/llvm/MC/MCInstrDesc.h | ||
---|---|---|
214 | Ah, many small steps make a great leap. | |
llvm/utils/TableGen/InstrInfoEmitter.cpp | ||
922 | I'll start digging and removing the rust. | |
1070 | 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. |
@foad MSVC builds seem to be breaking due to this:
https://lab.llvm.org/buildbot/#/builders/86/builds/53509
LLVMARMAsmParser.lib(ARMAsmParser.cpp.obj) : error LNK2001: unresolved external symbol "class llvm::MCInstrDesc const * const llvm::ARMDescs" (?ARMDescs@llvm@@3QBVMCInstrDesc@1@B) bin\llc.exe : fatal error LNK1120: 1 unresolved externals ninja: build stopped: subcommand failed.
Thanks - I just noticed that too. I have a quick fix but can't push it now due to some partial github outage.
Could we give OpInfo the same treatment?