TableGen support for RegisterBankInfo #71357

CBSears · 2023-11-06T05:42:47Z

This is a continuation of #70895

TableGen generates boilerplate code from definition files. It already has some RegisterBankInfo support: TargetGenRegisterBankInfo(), RegBankIDs enum, RegBanks and Sizes. But it is missing support for PartialMappingIdx and PartMappings. This adds support for that. It is discussed in

https://discourse.llvm.org/t/rfc-tablegen-support-for-registerbankinfo/

github-actions · 2023-11-06T05:54:03Z

✅ With the latest revision this PR passed the C/C++ code formatter.

rovka

Could you update the RegBankEmitter test?

llvm/utils/TableGen/RegisterBankEmitter.cpp

for RegisterBankInfo tables and enum

or untyped RegisterClass

Pierre-vh · 2023-11-08T06:59:46Z

llvm/utils/TableGen/RegisterBankEmitter.cpp

+                                   const std::vector<RegisterBank> &Banks);
+  void emitRBIHeader(raw_ostream &OS, const StringRef TargetName,
+                     const std::vector<RegisterBank> &Banks);
+  void emitRBIImplementation(raw_ostream &OS, const StringRef TargetName,
+                             const std::vector<RegisterBank> &Banks);


const StringRef is not really needed, just use StringRef
extra small nit: vector& could be ArrayRef but since other functions also use a ref it's ok to stay consistent with them.

Sure but there are other const StringRef uses. Shall I change them as well?

Up to you. Improving the existing code is always nice but if it's too much of a tangent for this patch I understand as well

Pierre-vh · 2023-11-08T07:01:16Z

llvm/utils/TableGen/RegisterBankEmitter.cpp

+// RegisterBankInfo tables and enums
+// are discussed in https://discourse.llvm.org/t/74459


There are 21 posts in that thread at the time of writing. Can you remove the link and just put a summary of what this does?

Pierre-vh · 2023-11-08T07:03:08Z

llvm/utils/TableGen/RegisterBankEmitter.cpp

+      if (RC->RSI.isSimple()) {
+        if (RC->getValueTypes()[0].getSimple() != MVT::Untyped) {
+          // StartIdx is currently 0 in all of the in-tree backends
+          OS << "  { 0, " << RC->RSI.getSimple().RegSize << ", "
+             << Bank.getInstanceVarName() << " },\n";
+        } else {
+          OS << "  #error Untyped RegisterClass " << RC->getName() << "\n";
+        }
+      } else {
+        OS << "  #error non-Simple() RegisterClass " << RC->getName() << "\n";
+      }


small nit: to reduce nesting, you could do !RC->RSI.isSimple() and continue afterwards.

Thanks. I went a little further and reversed both to avoid the continue. It reads a little more clearly and avoids the nesting.

Pierre-vh · 2023-11-08T07:03:41Z

llvm/test/TableGen/RegisterBankEmitter.td

+// CHECK: const PartialMappingIdx BankIDToFirstRegisterClassIdx
+// CHECK: PMI_ClassA,
+// CHECK: const int BankIDToRegisterClassCount
+// CHECK: 1,


newline at end of file

Sure. I didn't know about this.

CBSears · 2023-11-08T16:17:22Z

BTW, I may figure out ValueMappings yet and include generation for that.

arsenm · 2023-11-09T08:33:34Z

llvm/utils/TableGen/RegisterBankEmitter.cpp

+         Bank.getExplicitlySpecifiedRegisterClasses(RegisterClassHierarchy)) {
+      if (!RC->RSI.isSimple()) {
+        OS << "  #error non-Simple() RegisterClass " << RC->getName() << "\n";
+      } else if (RC->getValueTypes()[0].getSimple() == MVT::Untyped) {


Not sure you can assume a register class has any set types

The TableGen RegisterClass constructor has

if (TypeList.empty()) PrintFatalError(R->getLoc(), "RegTypes list must not be empty!");

BTW, this logic is to detect the AArch64 case and AArch64 XSeqPairsClass indeed has

def XSeqPairsClass : RegisterClass<"AArch64", [untyped], 64,

arsenm · 2023-11-09T08:39:15Z

llvm/utils/TableGen/RegisterBankEmitter.cpp

+      if (!RC->RSI.isSimple()) {
+        OS << "  #error non-Simple() RegisterClass " << RC->getName() << "\n";
+      } else if (RC->getValueTypes()[0].getSimple() == MVT::Untyped) {
+        OS << "  #error Untyped RegisterClass " << RC->getName() << "\n";


Single quotes for single characters

But "\n" is used elsewhere in this file and '\n' isn't. So I'm inclined to just follow their example.

'\n' calls a different operator<< which is usually inlined vs. one that usually isn't. "\n" is more expensive

llvm/utils/TableGen/RegisterBankEmitter.cpp

arsenm · 2023-11-09T08:53:12Z

llvm/utils/TableGen/RegisterBankEmitter.cpp

@@ -289,6 +299,141 @@ void RegisterBankEmitter::emitBaseClassImplementation(
     << "} // end namespace llvm\n";
 }

+// TableGen already had some RegisterBankInfo support:


Don't need the history lesson

Yeah, that was mostly for me. I'll simplify it.

topperc · 2023-11-12T05:03:40Z

llvm/utils/TableGen/RegisterBankEmitter.cpp

+  for (const auto &Bank : Banks) {
+    for (const CodeGenRegisterClass *RC :
+         Bank.getExplicitlySpecifiedRegisterClasses(RegisterClassHierarchy)) {
+      OS << "  PMI_" << RC->getName() << " = " << ID++ << ",\n";


My understanding is that a PartialMappingIdx is supposed to be a register bank and size not a register class. ARM and Mips name all of theirs using register class names. PowerPC uses more generic names like FPR32, FPR64, and VEC128.

The possible sizes for a register bank in the partial mapping index should be determined by the sizes of any register classes in the register bank. This should include the inferred register classes.

Because the sizes of the register classes determine the size for the partial mapping index, there is a correspondence.

@qcolombet @aemerson @dsandersllvm can you confirm my understanding?

PowerPC’s PartialMappingIdx in PPCRegBankInfo.h doesn’t match the Register Classes in PPCRegisterBanks.td

One of the things about having a TableGen emitter is that there will be some enforced standardization of naming. That standard could go one way or it could go another. But in the end, hopefully all of the backends follow the resulting convention which will make code sharing easier.

PowerPC’s PartialMappingIdx in PPCRegBankInfo.h doesn’t match the Register Classes in PPCRegisterBanks.td

That's because a partial mapping index is not a register class. It's a register bank and size.

It's supposed to describe the contents of the corresponding row in partial mapping table. Each row in the table is a register bank, a length, and a start index. The start index is always 0. The partial mapping index names should be the register bank and the length concatenated together. So they describe the row contents.

The RISC-V partial indices should be named GPRB32, GPRB64, FPRB32, and FPRB64

This is Mips:

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

I don't think it really matters which way as long as there's one consistent way. Right now, without an emitter laying down the law, the backends have wandered off in different directions. They compile and they are internally consistent but it makes sharing code harder.

I also don't think that when we pick one standard, that the resulting edits are going to be anything but an NFC.

I'm well aware that everything is inconsistent. I'm trying to explain what I believe AArch64 intended when they started this project.

I believe the intent of the register class lists in *RegisterBanks.td was to allow targets to list the minimum number of register classes to allow tablegen to find the rest of the related to registers. This is why RISC-V uses FPR64 only and AArch64 uses QQQQ. Those are the largest super classes that the FPR registers on those targets belong to. Tablegen can walk down the subregister information from there and find all of the register classes that need to belong to the bank. This is expressed in the CoverageData table. The type lists on the register classes should have nothing to do with this.

The union of the sizes from all of the covered register classes should be used to fill in the partial mapping table. Tblgen infers that the FPR register bank for RISC-V includes the following register classes

const uint32_t FPRRegBankCoverageData[] = { // 0-31 (1u << (RISCV::FPR64RegClassID - 0)) | (1u << (RISCV::FPR16RegClassID - 0)) | (1u << (RISCV::FPR32RegClassID - 0)) | (1u << (RISCV::FPR64CRegClassID - 0)) | (1u << (RISCV::FPR32CRegClassID - 0)) | 0, // 32-63 0, // 64-95 0, };

Examining the sizes of those 6 register classes will reveal 3 unique sizes, 16, 32, and 64. So RISC-V should have PMI_FPR16, PMI_FPR32, and PMI_FPR64 as partial mapping indices for the FPR bank. I'm aware that PMI_FPR16 is missing today. I will add it soon.

For GPR on RISC-V we there are many register classes but they all have the same sizes which comes from HwMode. I think we should walk through all of the register classes in GPRRegBankCoverageData and for each class look at the possible sizes HwMode gives for them. They should all give the same two values, 32 and 64. So we should have PMI_GPR32 and PMI_GPR64.

On AArch64, FPRRegBankCoverageData contains a bunch of register classes inferred by QQQQ. I bet if we check the sizes of the all of those classes we will find sizes 8, 16, 32, 64, 128, 256, and 512. All of the sizes for FPR in AArch64's partial mapping index for FPR. Except 8 which I think is just missing from GlobalISel.

The partial mapping should just be a size. However, I am not sure what the ultimate value of this level of detail provides. As it stands we have a ton of infrastructure to produce a bunch of register sizes, but then they're not actually useful to code that needs to use them.

RegBankSelect tries to pre-create registers for you with appropriate sizes, but that doesn't preserve the type if you're doing a vector breakdown for example. You also aren't free to just directly use the registers that were created for you, and it doesn't compose with constructing new instructions. For example if you try to split a 64-bit operation into operations on 2 32-bit pieces, you may end up with 2 32-bit registers but you ultimately need to figure out how to undo the split to produce a valid merge instruction

arsenm · 2024-04-03T20:48:00Z

Reverse ping? This needs an update to main

Chris Sears added 3 commits November 5, 2023 16:41

renamed emitRBIIMPL and other small changes

d6f25b0

added const keyword to emitted arrays

7bee10e

clang-format formatting

e74d925

CBSears mentioned this pull request Nov 6, 2023

add RegisterBankInfo support to RegisterBankEmitter #70895

Closed

fixing clang-format complaint

241919d

rovka reviewed Nov 6, 2023

View reviewed changes

llvm/utils/TableGen/RegisterBankEmitter.cpp Show resolved Hide resolved

Chris Sears added 6 commits November 6, 2023 10:10

Updating RegisterBankEmitter.td to include CHECKs

7ed514c

for RegisterBankInfo tables and enum

added comment pointing at discourse RFC

1f52ef1

Fixed clang-format complaint

ffe12e4

added more tests to RegisterBankEmitter.td

1280d9c

one more RegisterBankEmitter.td test

b5dc5f3

the emitter now writes #error lines if it detects either a non-Simple

f701940

or untyped RegisterClass

Pierre-vh reviewed Nov 8, 2023

View reviewed changes

Chris Sears added 4 commits November 8, 2023 08:17

resolving PR comments

6141ee2

Added support for ValueMappings

775e71e

fixed clang format complaint

18ff040

added RegisterBankEmitter test

4250c64

arsenm reviewed Nov 9, 2023

View reviewed changes

Improved comment.

baa94ec

topperc reviewed Nov 12, 2023

View reviewed changes

CBSears closed this by deleting the head repository May 22, 2025

		// RegisterBankInfo tables and enums
		// are discussed in https://discourse.llvm.org/t/74459

TableGen support for RegisterBankInfo #71357

TableGen support for RegisterBankInfo #71357

Uh oh!

Conversation

CBSears commented Nov 6, 2023

Uh oh!

github-actions bot commented Nov 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rovka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CBSears commented Nov 8, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CBSears Nov 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CBSears Nov 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arsenm commented Apr 3, 2024

Uh oh!

Uh oh!

github-actions bot commented Nov 6, 2023 •

edited

Loading

CBSears Nov 12, 2023 •

edited

Loading

CBSears Nov 12, 2023 •

edited

Loading