We have some unfortunate code in the back end that defines a bunch of register sets for the Asm Parser. Every time another class is needed in the parser, we have to add another one of those definitions with explicit lists of registers. This NFC patch simply provides macros to use to condense that code a little bit.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
We have similar definitions in PPCDisassembler.cpp, it would be good to condense those definitions as well.
Comment Actions
Good idea! Looks like most of these register sets are common for assembler/disassembler, can we do one step further to put all these in one include file? Or even better: generated that include file by TableGen! :)
| lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp | ||
|---|---|---|
| 62 ↗ | (On Diff #173713) | Looks like that it would be even better if we can generate all these by TableGen. DisassemblerEmitter.cpp is already generating differently for X86/WebAssembly/ARM, it should be fair for us to add PPC specific logic there. |
Comment Actions
This is how I meant to do it initially but was reluctant to introduce a new header dependency. However, I don't think it's an issue at all for PPCMCTargetDesc.h to have a dependency on MC/MCRegisterInfo.h.
| lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp | ||
|---|---|---|
| 62 ↗ | (On Diff #173713) | We can look at doing this subsequently, but I believe that PPC has a bit of a unique situation in terms of complexity of register classes. So we would likely just be reimplementing the same thing in TableGen and it would only be used on PPC.
|
| lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp | ||
|---|---|---|
| 62 ↗ | (On Diff #173713) | OK for looking at this later. |
| lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h | ||
| 136 ↗ | (On Diff #173812) | PPCMCTargetDesc.h is included in several files, eg: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp, llvm/lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp, llvm/lib/Target/PowerPC/MCTargetDesc/PPCMachObjectWriter.cpp.. Declaring static in this header will introduce unnecessary and useless copies o those modules. Maybe we should put these in a new file that included only in necessary modules? |
| lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h | ||
|---|---|---|
| 136 ↗ | (On Diff #173812) | We can certainly do that. However, these are static so they will be optimized out of any CU's that don't have any uses of them. So I'm not sure what you mean by copies. Are you concerned about increasing compile time when compiling those CU's (in order to determine these arrays aren't needed)? |
| lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h | ||
|---|---|---|
| 136 ↗ | (On Diff #173812) | Yes, static *can* be optimized out by build compiler, but that is up to the build env, not by language standard. I am not worrying about the extra compile time, and in this specific situation , it maybe fine for now. but I still don't think it is a good practice to put static variables in headers. static variables in headers DOES introduce unnecessary "global" variables to other cpp files, which increase the chance of naming collision, memory allocation (at no-opt) and it also impose "requirement" for build env: the files should be built at opt, and compiler should have the capability to optimize unused arrays out. I would prefer we put those into a .inc file, and include them only when we do need these arrays. |
Comment Actions
Removed the actual definitions of the static arrays from the header. Simply provided the macro for the defs in the header and the macro is used in only the two CU's that need it.
This way if we need to add any new ones, they just need to be added to the macro.
Comment Actions
LGTM.
Although it is easy to make error-prone code in macros,
it is much better than having similar code in multiple files.
A great step forward before we reimplementing them in TableGen.
Thanks for fixing!