[RFC] TableGen support for RegisterBankInfo

Code Generation Common Infrastructure
1

[RFC] TableGen support for RegisterBankInfo

Introduction

TableGen generates boilerplate code from definition files. It already has some RegisterBankInfo support: TargetGenRegisterBankInfo(), RegBankIDs enum, RegBanks and Sizes. But it’s missing support for PartMappings, PartialMappingIdx and BankIDToCopyMapIdx. FWIW, I don’t think there’s enough TableGen information to emit ValMappings and ValueMappingIdx which are used in PowerPC, X86, AMDGPU and AArch64.

Current State

Currently, in order to provide the tables and enumerations that GlobalISel needs, this derivable code is written manually. Later, this code might even be copied and pasted into new backends and then modified, also manually. Sometimes this code is separated out into .def or .h files and sometimes it is just embedded directly into a target backend’s .cpp files. Nonstandard naming has crept in. The more custom code the backends adopt, the harder it will be to share code and develop utility routines and infrastructure.

Proposal

This all needs a cleanup which will not happen without some sturm und drang (encoding holes, subregs, …). However, the result will be cleaner existing GlobalISel implementations and will make it easier to write new backends.

Again, RegisterBankEmitter.cpp already has some RegisterBankInfo support. So the missing support can be added without needing any new source files, any new TargetRegisterBankInfo.inc files or changes to any of the CMakeLists.txt files. A target which needs RegisterBankInfo support already has RegisterBank support and consequently it already has a CMake rule (-gen-register-bank) that generates a TargetRegisterBank.inc file; the current RegisterBankInfo support piggybacks on RegisterBank. So the generated TargetRegisterBank.inc file now gets included 4 times rather than just 3.

There will be a new macro GET_TARGET_REGBANKINFO_DECLARATIONS which is necessary to control what gets included where.

This change won’t break any out-of-tree backends which may already have their own custom RegisterBankInfo support. Backends using this new support will have to adhere to the new TableGen conventions but those backends can adapt to the new regime at their convenience. In fact, we should be able to commit the new RegisterBankEmitter without any functional changes whatsoever. The in-tree backends can simply adapt the same as the out-of-tree backends, at their convenience. Of course, we’ll have to adapt one backend for testing but its changes won’t even have to be committed initially.

The current RegisterBankInfo support doesn’t clash with anything being proposed here and I expect to leave it alone. But at some point, the current support could be moved inside the REGBANKINFO #ifdefs for better readability.

Risks

The ordering of indices and tables will change. For example, the ARM backend currently has:

enum PartialMappingIdx {
  PMI_GPR,
  PMI_SPR,
  PMI_DPR,
  PMI_Min = PMI_GPR,
};

which is manually created (at the same time ignoring GPRwithAPSR, HPR and QPR) from:

def GPRRegBank : RegisterBank<"GPRB", [GPR, GPRwithAPSR]>;
def FPRRegBank : RegisterBank<"FPRB", [HPR, SPR, DPR, QPR]>;

but will be generated by TableGen as:

enum PartialMappingIdx {
  PMI_None = -1,
  PMI_HPR = 0,
  PMI_SPR = 1,
  PMI_DPR = 2,
  PMI_QPR = 3,
  PMI_GPR = 4,
  PMI_GPRwithAPSR = 5,
};

Banks and RegisterClasses are not provided by TableGen in their original text order and so backends can’t depend on that or any particular total order. PartialMappingIdx is only partially ordered: all RCs of a bank are contiguous but in no particular order and Banks are in no particular order. BankIDToCopyMapIdx has the PartialMappingIdx of the first RC of each Bank.

PartialMappingIdx BankIDToCopyMapIdx[] = {
  PMI_HPR,
  PMI_GPR,
};

Refactoring

We can make an effort to factor out some of the utility routines which have been written in the various RegisterBankInfo implementations. These would be inside the GET_TARGET_REGBANKINFO_IMPL #ifdef where target backends can use them or ignore them. There will be no naming collisions unless the target defines GET_TARGET_REGBANKINFO_IMPL and includes the source. But really, anything interesting should probably get moved to the RegisterBankInfo base class.

Questions

Why is PMI_Min necessary? It seems especially unnecessary for TableGen generated enumerations which won’t have any encoding holes. PMI_Min is used in M68k, Mips, PowerPC and ARM where it’s 0 and also in AArch64 where it’s 7 (PMI_GPR32). It seems this AArch64 code was probably copied from AArch64 into the other backends where it’s just bloat.

Why is PMI_FPR16 == 1? Indeed, PartialMappingIdx in AArch64 is irregular. AMDGPU does this as well. Does AArch64 and AMDGPU really need this irregularity? This irregularity doesn’t appear in the X86 backend.

Why don’t AArch64 floating point registers have a register bank?

Why is AMDGPU PartialRegisterIdx prefix PM rather than PMI? Regularity makes it easier to compare code across GlobalISel implementations and TableGen helps enforce this regularity.

StartIdx? StartIdx is manually set to 0 in all of the backends. Is it that value the same as RC->RSI.getSimple().SpillAlignment?

CRRegBank needs to map to PMI_None in BankIDToCopyMapIdx. This would be simpler if CRRC was marked “let isAllocatable = 0;” or some other marking.

SubRegs?

RISCV is complicated because of def XLenVT : ValueTypeByHwMode<[RV32, RV64], [i32, i64]>; So getSimple().RegSize crashes. We need a way to iterate through HwModes. This is an issue with BankIDToCopyMapIdx

PowerPC’s PartialMappingIdx in PPCRegBankInfo.h doesn’t match the Register Classes in PPCRegisterBanks.td. BankIDToCopyMapIdx is used in PPC and AArch64, getCopyMapping().

2

After digging back through commit history it looks like AArch64 may start at 1 just to prove that the starting value doesn’t matter. See description from ⚙ D27337 [globalisel][aarch64] Fix unintended assumptions about PartialMappingIdx. NFC.

3

I don’t think is 7 on AArch64. It should be 1.

PMI_Min = PMI_FirstFPR → PMI_FirstFPR = PMI_FPR16 → PMI_FPR16 = 1

4

This is REGBANKINFO for ARM. I’m out the door just now but I’ll test this with the ARM backend Sunday night. BankIDToCopyMapIdx probably needs some explanation and maybe changes. Also, I’m going to call in to Quentin Colombet’s office hours on Wednesday to talk about this. Hopefully, ARM will work and I’ll push the code before then.

#ifdef GET_REGBANKINFO_DECLARATIONS
#undef GET_REGBANKINFO_DECLARATIONS
namespace llvm {
namespace ARM {
enum PartialMappingIdx {
  PMI_None = -1,
  PMI_HPR = 0,
  PMI_SPR = 1,
  PMI_DPR = 2,
  PMI_QPR = 3,
  PMI_GPR = 4,
  PMI_GPRwithAPSR = 5,
};

RegisterBankInfo::PartialMapping PartMappings[] {
  { 0, 16, FPRRegBank },
  { 0, 32, FPRRegBank },
  { 0, 64, FPRRegBank },
  { 0, 128, FPRRegBank },
  { 0, 32, GPRRegBank },
  { 0, 32, GPRRegBank },
};

PartialMappingIdx BankIDToCopyMapIdx[] {
  PMI_HPR,
  PMI_GPR,
};
} // end namespace ARM
} // end namespace llvm
#endif // GET_REGBANKINFO_DECLARATIONS
5

RISCV describes one GPR register bank in its RISCVRegisterBank.td file

def GPRRegBank : RegisterBank<"GPRB", [GPR]>;
def FPRRegBank : RegisterBank<"FPRB", [FPR64]>;

but expects 32b and 64b variants (RV32 and RV64) in PartialMappingIdx and PartMappings.

enum PartialMappingIdx {
  PMI_GPR32 = 0,
  PMI_GPR64 = 1
};

RegisterBankInfo::PartialMapping PartMappings[] = {
    {0, 32, GPRRegBank},
    {0, 64, GPRRegBank}
};

This requires getting information from outside of the GISel TableGen descriptions and RISCV is alone in doing this.

According to the GISel documentation, the definition of a RegisterBank is “rather loose”. It would be a LOT easier for RegisterBankEmitter if we take advantage of this looseness and for RISCVRegisterBank.td to just have GPR32 and GPR64. This might duplicate some HwMode information. We’ll need to have the RISCV people look at this. There’s also RISCVTargetDefEmitter (but which doesn’t seem to be used (yet?); there is no RISCVTargetDefEmitter::run() in TableGen or -gen-riscv-target-def in the backend).

It’s not clear why some backends are including condition code registers (for example, AArch64 CC) in the Register Banks. These registers will never be allocated by the register allocator. These registers should be marked “let isAllocatable = 0;”.

On the other hand, m68k doesn’t describe its floating point registers in Register Banks.

6

I commented out the manually written code GISel enums+tables in the ARM backend and spliced ARMGenRegBank.inc in. Then I ran the lit tests and they passed. The minor additional change in ARMRegisterBankInfo.cpp was getting rid of PMI_Min (which was 0).

% path/llvm-lit  another_path/llvm/test/CodeGen/ARM
...
Testing Time: 449.21s
  Passed           : 1497
  Expectedly Failed:    2

So I’ll push the RegisterBankEmitter.cpp code tonight. It’s very short.

7

Thanks for finally fixing this! It will be nice to be free of those unmanageable tables. Still wondering if this level of detail is useful though… The regbankselect APIs still don’t really make much sense.

8

+1 on all that.

This work would be a welcome improvement but before investing too much time into this, I think it would make sense to step back and check if the abstractions need to be reworked.

@arsenm could you share what the new abstraction should look like?

9

This cleanup is not a lot of work. It’s maybe 100 LOC in RegisterBankEmitter.cpp. On the TableGen side, it’s already done. On the backend side, I’ve tested it with the ARM backend and it’s mostly deleting the manual code and putting in a couple of include statements. I say mostly. Without TableGen enforcing a standard, the backends have diverged a little.

As for improving the API abstraction, yes, but I think that’s a second phase after this cleanup and then I’m probably not the right person to propose that design.

BTW, I should have pushed this yesterday but I just use git locally and I’ve had some trouble figuring out GitHub. LLVM needs a GitHub onboarding doc the way they’ve had one with Phabricator.

1 Like
10

You mean like LLVM GitHub User Guide — LLVM 18.0.0git documentation (linked from Getting Involved — LLVM 18.0.0git documentation)?

11

BTW, I’m not changing the API at all. That said, the enums and tables are slightly different. Basically all of the RegisterBank and RegisterClass data is now there and you can iterate over it. I think this should help with any new API abstraction.

From before, there is already the enumeration of RegisterBankIDs, NumRegisterBanks, RegBanks and Sizes.

TableGen now generates all RegisterBanks and PartialMappingIdx+PartMappings for all of the RegisterClasses. Some of the manually written code omits some of the RegisterClasses.

Lastly, PowerPC and AArch64 have an array BankIDToCopyMapIdx which looks like:

PPCGenRegisterBankInfo::PartialMappingIdx
  PPCGenRegisterBankInfo::BankIDToCopyMapIdx[]{
    PMI_None,
    PMI_FPR64,  // FPR
    PMI_GPR64,  // GPR
    PMI_VEC128, // VEC
};

Note that it has a PMI_None entry and no entry for CR.

Condition code registers get RegisterBank entries in the TableGen def files but these RegisterClasses should be marked as non-allocable. Sounds obvious unless you’re writing TableGen emitter code.

Ok, I might get yelled at for this but I’ve omitted PMI_None, changed the name to BankIDToFirstRegisterClassIdx and added BankIDToRegisterClassCount. These look like (for ARM):

PartialMappingIdx BankIDToFirstRegisterClassIdx[] = {
  PMI_HPR,
  PMI_GPR,
};

int BankIDToRegisterClassCount[] = {
  4,
  2,
};

So while I haven’t changed the API, it is now possible to iterate over RegisterBanks and their RegisterClasses. I don’t know that this is useful but it seemed so so close to what PowerPC and AArch64 were doing with BankIDToFirstRegisterClassIdx. I did this without understanding the ValueMapping code.

Mind you, this isn’t forcing AArch64 or PowerPC to do anything. They can ignore this and even still use the other TableGen’d data. I just thought it would be generally useful. Besides, I do not see a way of generating their ValueMapping arrays from the available TableGen defs.

12

Yeah, that’s a User Guide (reference) rather than an Onboarding Document (tutorial). Its first paragraph is:

Do not create any branches in the llvm/llvm-project repository. This repository is reserved for official project branches only. We may relax this rule in the future if needed to support “stacked” pull request, but in that case only branches being used for “stacked” pull requests will be allowed.

None of that made any sense to me. It’s a good reference doc but it still needs a good tutorial which is a different kind of document assuming a different level of reader understanding.

13

I’ll let you know if I ever have time to figure out what it should be :slight_smile:

14

I’ve submitted the pull request.

  1. backends delete their manual code and include TableGen code
  2. backends cleanup/regularize their TableGen defs
    AArch64 no longer lists all of their RegisterBanks and Classes
  3. where do we get StartIdx from? RC->RSI.getSimple().SpillAlignment?
    it’s 0 in all of the in-tree backends
  4. RISCV is non Simple() because
    def XLenVT : ValueTypeByHwMode<[RV32, RV64], [i32, i64]>;
  5. no more PMI_Min and everything is 0 based
  6. AMDGPU PartialRegisterIdx prefix PM rather than PMI
  7. CRRC (condition code registers) should be marked “let isAllocatable = 0;”
  8. How are subregs handled?
  9. PowerPC’s PartialMappingIdx in PPCRegBankInfo.h doesn’t match the Register Classes in PPCRegisterBanks.td
  10. m68k doesn’t describe its floating point registers
15

Sounds like submitted your PR is indeed the right move at this point.

Thanks for the clean-up!

16

The point of this TableGen support is to clean up some accumulated cruft. In a very real sense it will represent no functional change even if it may not qualify for the NFC tag.

This RegBankInfo cleanup will happen in two phases. The first phase is just RegisterBankEmitter itself. Get some backend feedback. Review the code. Commit it. Since its emitted code is protected behind GET_REGBANKINFO_* ifdefs, backends won’t break. The status quo will require no backend changes. Yet.

The second phase of individual backend commits is considerable, to actually rework the in-tree backends to use the new TargetGenRegisterBank.inc files with the TableGen’d PartMappings, PartialMappingIdx and BankIDToCopyMapIdx. This will include:

  • deleting the old manually written code
  • insert define + include
  • renaming some variables (AMDGPU has PM_ instead of PMI_)
  • 0 based enums
  • cleaning up TableGen records
  • moving some code elsewhere (AArch64GenRegisterBankInfo.def)
  • deleting some files
  • testing: compile + codegen
  • GISel directory? (X86, MIPS, SPIRV, ARM, AMDGPU)

I’ve tested this all with the ARM backend and it passed the codegen tests. I’ll join the RISC-V, PowerPC and Arch64 sync-up calls. My biggest problems right now are with RISC-V and AArch64.

I still can’t find a way to generate ValueMappings and ValueMappingIdx from the existing TableGen data. Maybe someone else has a clue. Maybe we need another TableGen record encoding the same information. But for right now, I recommend just moving those tables and enums into TargetRegisterBankInfo.cpp

There are 9 in-tree GlobalISel backends: AArch64, PowerPC, AMDGPU, RISCV, ARM, SPIRV, M68k, X86 and Mips. All of them generate TargetGenRegisterBank.inc files.

AArch64 doesn’t explicitly list its RegisterClasses using XSeqPairsClass instead: def GPRRegBank : RegisterBank<"GPR", [XSeqPairsClass]>;. AArch64 is alone is using XSeqPairsClass which makes it really hard for TableGen.

RISCV doesn’t list its floating point RegisterClasses at all and uses
another TableGen technique to derive 64+32 bit register banks from a kind of virtual register class definition: def GPRRegBank : RegisterBank<"GPRB", [GPR]>; Similarly, this virtual/template makes it hard for TableGen.

M68k doesn’t list its floating point RegisterClass

X86 doesn’t list all of its RegisterClass

Standardizing and making everything explicit in the .td files will make life easier for TableGen and easier for backend developers to compare and share code.

1 Like
17

I’m having some GitHub problems so I closed #70895 and re-opened it as #71357.

TableGen support for RegisterBankInfo #71357

Clang-Format complained and so there’s an updated commit that I can’t approve.

Otherwise, I think this first part, RegisterBankEmitter, is ready for review.

18

Why is PMI_Min necessary?

IIRC this was to protect against enums where the first valid entry would not be 0.
E.g., the entry with 0 could have been PMI_None, although today I believe we use -1 for this one.

In any case, if the table is Tablegen’ed, we probably don’t need this anymore.

@dsanders, do you remember the context here by any chance?

Why is PMI_FPR16 == 1?

I think it was just to test that we don’t assume that the first PMI is 0.

Does AArch64 and AMDGPU really need this irregularity?

For AArch64, I would be surprised if anything breaks if we change it to 0.
For AMDGPU, I’d say it doesn’t need this irregularity, but it won’t work out-of-the-box if we change this index to 0.
The reason is index 0 is used for the VCC register bank and this index is currently hardcoded, AFAICT. In other words, if VCC had an entry in PMI, it should be the one mapped to 0 to match the current layout of the partial mappings.

Why don’t AArch64 floating point registers have a register bank?

What do you mean?
AArch64 floating point registers have a register bank: AArch64::FPRRegBank.
It is defined in llvm/lib/Target/AArch64/AArch64RegisterBanks.td.

Why is AMDGPU PartialRegisterIdx prefix PM rather than PMI?

No idea. I’d assume we can use PMI for all the backends.

StartIdx? StartIdx is manually set to 0 in all of the backends. Is it that value the same as RC->RSI.getSimple().SpillAlignment?

Which StartIdx is that?
I.e., where do you see it?

CRRegBank needs to map to PMI_None in BankIDToCopyMapIdx. This would be simpler if CRRC was marked “let isAllocatable = 0;” or some other marking.

Which backend is that?
For AArch64, I believe this is already the case.

SubRegs?

What about them?

RISCV is complicated because of def XLenVT : ValueTypeByHwMode<[RV32, RV64], [i32, i64]>; So getSimple().RegSize crashes. We need a way to iterate through HwModes. This is an issue with BankIDToCopyMapIdx

Haven’t thought about it. Let me know if some brainstorming is needed.

PowerPC’s PartialMappingIdx in PPCRegBankInfo.h doesn’t match the Register Classes in PPCRegisterBanks.td. BankIDToCopyMapIdx is used in PPC and AArch64, getCopyMapping().

Someone using PowerPC would have to comment.

19

Why don’t AArch64 floating point registers have a register bank?

Ok, so I understand this now. def FPRRegBank : RegisterBank<"FPR", [QQQQ]>; Going back to Justin Bogner’s talk, he said the generic code will walk sub register classes. However, getExplicitlySpecifiedRegisterClasses() (which isn’t the code he was talking about) doesn’t do that. It only walks the Explicitly Specified Register Classes. Since the generic code was ever finished, the backends then marched off in different directions each solving this problem in their own way.

AArch64 just says QQQQ and has a SetTheory based walker which wasn’t finished. So they hand craft PartMappings while explicitly specifying the Register Classes. RISCV says GPR and ValueTypeByHWMode which wasn’t finished. X86 is a problem in itself because of its subregisters. M68k follows RISCV but it doesn’t need the level of generality it’s copying; so maybe it could be converted to instead use explicitly specified registers.

On the other hand, AMDGPU explicitly specifies its Register Classes and so TableGen can emit enums and a table. Yes, those are written out in a different order (than text order) because of hashing but they should work. Same with ARM. Same with Mips. Same with PowerPC. Same with SPIRV.

So I think we can quickly adapt 5 of the 9 GISel backends. Maybe 6. However AArch64, RISCV and X86 are each different problems and we’ll have to get their developers involved. Indeed, it may be too much work for the benefit.

Needless to say, I strongly prefer explicitly specified registers because it makes TableGen’s life easier and it documents the register structure in the TargetRegisterBanks.td rather than in the code.

20

In fact, M68k does work with the emitter. Also, X86 doesn’t fail but then that’s only because it doesn’t really describe much with its X86RegisterBanks.td file. Other backends may not fully describe their register banks. But that’s an easier problem to deal with.

So 7 of the 9 in-tree GISel backends should now work with only simple changes, leaving only AArch64 and RISCV. I’ve added some code to recognize their use cases and then emit #error lines into the TargetGenRegisterBank.inc files. This should help any of the out of tree backends which had followed their lead.

const RegisterBankInfo::PartialMapping PartMappings[] = {
  { 0, 32, CCRegBank },
  #error Untyped RegisterClass QQQQ
  #error Untyped RegisterClass XSeqPairsClass
};
const RegisterBankInfo::PartialMapping PartMappings[] = {
  { 0, 64, FPRRegBank },
  #error non-Simple() RegisterClass GPR
};