This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][NFC] Macro for register set defs for the Asm Parser
ClosedPublic

Authored by nemanjai on Nov 12 2018, 9:49 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

nemanjai created this revision.Nov 12 2018, 9:49 AM

We have similar definitions in PPCDisassembler.cpp, it would be good to condense those definitions as well.

nemanjai updated this revision to Diff 173713.Nov 12 2018, 10:31 AM

Did the same for the disassembler.

jsji added a comment.Nov 12 2018, 10:57 AM

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
64

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.

nemanjai updated this revision to Diff 173812.Nov 12 2018, 8:21 PM

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.

nemanjai added inline comments.
lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp
64

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.
There are arrays that are nearly what we want generated by TableGen, but those have two major issues with them:

  • Allocation order affects initialization order of the array
  • The arrays may contain arrays we don't want to assign in the assembler/disassembler (frame pointer, base pointer, ...)
jsji added inline comments.Nov 13 2018, 9:15 AM
lib/Target/PowerPC/Disassembler/PPCDisassembler.cpp
64

OK for looking at this later.

lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h
136

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?

nemanjai added inline comments.Nov 14 2018, 12:34 PM
lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h
136

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)?

jsji added inline comments.Nov 14 2018, 1:26 PM
lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.h
136

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.

nemanjai updated this revision to Diff 174434.Nov 16 2018, 1:01 PM

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.

jsji accepted this revision.Nov 16 2018, 1:34 PM

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!

This revision is now accepted and ready to land.Nov 16 2018, 1:34 PM
This revision was automatically updated to reflect the committed changes.