[PATCH] D54433: [PowerPC][NFC] Macro for register set defs for the Asm Parser

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h:136
+
+static const MCPhysReg RRegs[32] = PPC_REGS0_31(llvm::PPC::R);
+static const MCPhysReg XRegs[32] = PPC_REGS0_31(llvm::PPC::X);
----------------
nemanjai wrote:
> jsji wrote:
> > 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?
> 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)?
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.






Repository:
  rL LLVM

https://reviews.llvm.org/D54433