[globalisel] Move as much RegisterBank initialization to the constructor as possible

[dsanders]

Summary

The register bank is now entirely initialized in the constructor. However,
we still have the hardcoded number of register classes which will be
dealt with in the TableGen patch (D27338) since we do not have access
to this information to resolve this at this stage. The number of register
classes is known to the TRI and to TableGen but the RegisterBank
constructor is too early for the former and too late for the latter.
This will be fixed when the data is tablegen-erated.

[rovka]

LGTM with microscopic nits.

include/llvm/CodeGen/GlobalISel/RegisterBank.h

46

It's probably worthwhile to add the comment explaining why this is necessary here (instead of only in the commit message).

lib/CodeGen/GlobalISel/RegisterBank.cpp

25

Incomplete comment :)

lib/Target/AArch64/AArch64RegisterBankInfo.cpp

45

I think you should move the asserts checking that getRegBank returns the expected banks before this loop (I can see how this will make the loop a bit more "buried", but it seems like the proper thing to do).

[dsanders]

Fix incomplete comment.
Add comment about finishInit()
Check getRegBank() works before relying on it to finish initialisation().

[qcolombet]

lib/CodeGen/GlobalISel/RegisterBank.cpp

30

We shouldn't need the TRI to do that.
By construction, we already have a limit of 64, but we will refine by making the tablegen backend smarter.

Indeed, eventually, TableGen shouldn't need this method, otherwise we missed something.
In particular, it is not unlikely that the TableGen backend will need to process the TargetRegisterInfo.td file with the TargetRegisterBank.td.

That should be automatic as I expect something like this in the XXXRegisterBank.td:

include XXXRegisterInfo.td

def MyBank : RegisterBank<(list of register classes)>

[dsanders]

Rebase and support ARM

[dsanders]

Rebased onto new D27808.

Finished moving to TargetRegisterClass initialization style.

RegisterBanks are now expected to be valid immediately after construction and
finishInit() is no longer needed.

[qcolombet]

LGTM

[dsanders]

Refresh before commit

include/llvm/CodeGen/GlobalISel/RegisterBank.h

private:
   /// Sentinel value used to recognize register bank not properly
   /// initialized yet.
   static const unsigned InvalidID;
 
   /// Only the RegisterBankInfo can initialize RegisterBank properly.
   friend RegisterBankInfo;
 
 public:
-  /// The default constructor will leave the object in
-  /// an invalid state. I.e. isValid() == false.
-  /// The fields must be updated to fix that and only
-  /// RegisterBankInfo instances are allowed to do that
-  RegisterBank();
+  RegisterBank(unsigned ID, const char *Name, unsigned Size,
+               const uint32_t *ContainedRegClasses);
 
   /// Get the identifier of this register bank.
   unsigned getID() const { return ID; }
 
   /// Get a user friendly name of this register bank.
   /// Should be used only for debugging purposes.
   const char *getName() const { return Name; }
 
   /// Get the maximal size in bits that fits in this register bank.

include/llvm/CodeGen/GlobalISel/RegisterBankInfo.h

protected:
   /// when GlobalISel is not built.
   /// That way, targets can still inherit from this class without doing
   /// crazy gymnastic to avoid link time failures.
   /// \note That works because the constructor is inlined.
   RegisterBankInfo() {
     llvm_unreachable("This constructor should not be executed");
   }
 
-  void setRegBankData(unsigned ID, const char *Name, unsigned Size,
-                      const uint32_t *CoveredClasses);
-
   /// Get the register bank identified by \p ID.
   RegisterBank &getRegBank(unsigned ID) {
     assert(ID < getNumRegBanks() && "Accessing an unknown register bank");
     return *RegBanks[ID];
   }
 
   /// Try to get the mapping of \p MI.
   /// See getInstrMapping for more details on what a mapping represents.

lib/CodeGen/GlobalISel/RegisterBank.cpp

 #include "llvm/Target/TargetRegisterInfo.h"
 
 #define DEBUG_TYPE "registerbank"
 
 using namespace llvm;
 
 const unsigned RegisterBank::InvalidID = UINT_MAX;
 
-RegisterBank::RegisterBank() : ID(InvalidID), Name(nullptr), Size(0) {}
+RegisterBank::RegisterBank(unsigned ID, const char *Name, unsigned Size,
+                           const uint32_t *CoveredClasses)
+    : ID(ID), Name(Name), Size(Size) {
+  ContainedRegClasses.resize(200);
+  ContainedRegClasses.setBitsInMask(CoveredClasses);
+}
 
 bool RegisterBank::verify(const TargetRegisterInfo &TRI) const {
   assert(isValid() && "Invalid register bank");
   for (unsigned RCId = 0, End = TRI.getNumRegClasses(); RCId != End; ++RCId) {
     const TargetRegisterClass &RC = *TRI.getRegClass(RCId);
 
     if (!covers(RC))
       continue;
     // Verify that the register bank covers all the sub classes of the
     // classes it covers.
 

lib/CodeGen/GlobalISel/RegisterBankInfo.cpp

 
 //------------------------------------------------------------------------------
 // RegisterBankInfo implementation.
 //------------------------------------------------------------------------------
 RegisterBankInfo::RegisterBankInfo(RegisterBank **RegBanks,
                                    unsigned NumRegBanks)
     : RegBanks(RegBanks), NumRegBanks(NumRegBanks) {
 #ifndef NDEBUG
-  for (unsigned Idx = 0, End = getNumRegBanks(); Idx != End; ++Idx)
+  for (unsigned Idx = 0, End = getNumRegBanks(); Idx != End; ++Idx) {
     assert(RegBanks[Idx] != nullptr && "Invalid RegisterBank");
+    assert(RegBanks[Idx]->isValid() && "RegisterBank should be valid");
+  }
 #endif // NDEBUG
 }
 
 RegisterBankInfo::~RegisterBankInfo() {
   for (auto It : MapOfPartialMappings)
     delete It.second;
   for (auto It : MapOfValueMappings)
     delete It.second;
 }
 
 bool RegisterBankInfo::verify(const TargetRegisterInfo &TRI) const {
 #ifndef NDEBUG
   for (unsigned Idx = 0, End = getNumRegBanks(); Idx != End; ++Idx) {
     const RegisterBank &RegBank = getRegBank(Idx);
     assert(Idx == RegBank.getID() &&
            "ID does not match the index in the array");
     DEBUG(dbgs() << "Verify " << RegBank << '\n');
     assert(RegBank.verify(TRI) && "RegBank is invalid");
   }
 #endif // NDEBUG
   return true;
 }
 
-void RegisterBankInfo::setRegBankData(unsigned ID, const char *Name,
-                                      unsigned Size,
-                                      const uint32_t *CoveredClasses) {
-  RegisterBank &RB = getRegBank(ID);
-  RB.ID = ID;
-  RB.Name = Name;
-  RB.Size = Size;
-  RB.ContainedRegClasses.resize(200);
-  RB.ContainedRegClasses.setBitsInMask(CoveredClasses);
-}
-
 const RegisterBank *
 RegisterBankInfo::getRegBank(unsigned Reg, const MachineRegisterInfo &MRI,
                              const TargetRegisterInfo &TRI) const {
   if (TargetRegisterInfo::isPhysicalRegister(Reg))
     return &getRegBankFromRegClass(*TRI.getMinimalPhysRegClass(Reg));
 
   assert(Reg && "NoRegister does not have a register bank");
   const RegClassOrRegBank &RegClassOrBank = MRI.getRegClassOrRegBank(Reg);

lib/Target/AArch64/AArch64GenRegisterBankInfo.def

const uint32_t CCRCoverageData[] = {
     // Classes 129-160
     0,
     // Classes 161-192
     0,
     // Classes 193-224
     0,
 };
 
-RegisterBank GPRRegBank;
-RegisterBank FPRRegBank;
-RegisterBank CCRRegBank;
+RegisterBank GPRRegBank(AArch64::GPRRegBankID, "GPR", 64, GPRCoverageData);
+RegisterBank FPRRegBank(AArch64::FPRRegBankID, "FPR", 512, FPRCoverageData);
+RegisterBank CCRRegBank(AArch64::CCRRegBankID, "CCR", 32, CCRCoverageData);
 
 RegisterBank *RegBanks[] = {&GPRRegBank, &FPRRegBank, &CCRRegBank};
 
 // PartialMappings.
 enum PartialMappingIdx {
   PMI_None = -1,
   PMI_GPR32 = 1,
   PMI_GPR64,

lib/Target/AArch64/AArch64RegisterBankInfo.cpp

AArch64RegisterBankInfo::AArch64RegisterBankInfo(const TargetRegisterInfo &TRI)
   // We have only one set of register banks, whatever the subtarget
   // is. Therefore, the initialization of the RegBanks table should be
   // done only once. Indeed the table of all register banks
   // (AArch64::RegBanks) is unique in the compiler. At some point, it
   // will get tablegen'ed and the whole constructor becomes empty.
   if (AlreadyInit)
     return;
   AlreadyInit = true;
-  // The GPR register bank is fully defined by all the registers in
-  // GR64all + its subclasses.
-  setRegBankData(AArch64::GPRRegBankID, "GPR", 64, AArch64::GPRCoverageData);
+
   const RegisterBank &RBGPR = getRegBank(AArch64::GPRRegBankID);
   (void)RBGPR;
   assert(&AArch64::GPRRegBank == &RBGPR &&
          "The order in RegBanks is messed up");
+
+  const RegisterBank &RBFPR = getRegBank(AArch64::FPRRegBankID);
+  (void)RBFPR;
+  assert(&AArch64::FPRRegBank == &RBFPR &&
+         "The order in RegBanks is messed up");
+
+  const RegisterBank &RBCCR = getRegBank(AArch64::CCRRegBankID);
+  (void)RBCCR;
+  assert(&AArch64::CCRRegBank == &RBCCR &&
+         "The order in RegBanks is messed up");
+
+  // The GPR register bank is fully defined by all the registers in
+  // GR64all + its subclasses.
   assert(RBGPR.covers(*TRI.getRegClass(AArch64::GPR32RegClassID)) &&
          "Subclass not added?");
   assert(RBGPR.getSize() == 64 && "GPRs should hold up to 64-bit");
 
   // The FPR register bank is fully defined by all the registers in
   // GR64all + its subclasses.
-  setRegBankData(AArch64::FPRRegBankID, "FPR", 512, AArch64::FPRCoverageData);
-
-  const RegisterBank &RBFPR = getRegBank(AArch64::FPRRegBankID);
-  (void)RBFPR;
-  assert(&AArch64::FPRRegBank == &RBFPR &&
-         "The order in RegBanks is messed up");
   assert(RBFPR.covers(*TRI.getRegClass(AArch64::QQRegClassID)) &&
          "Subclass not added?");
   assert(RBFPR.covers(*TRI.getRegClass(AArch64::FPR64RegClassID)) &&
          "Subclass not added?");
   assert(RBFPR.getSize() == 512 &&
          "FPRs should hold up to 512-bit via QQQQ sequence");
 
-  // Initialize the CCR bank.
-  setRegBankData(AArch64::CCRRegBankID, "CCR", 32, AArch64::CCRCoverageData);
-  const RegisterBank &RBCCR = getRegBank(AArch64::CCRRegBankID);
-  (void)RBCCR;
-  assert(&AArch64::CCRRegBank == &RBCCR &&
-         "The order in RegBanks is messed up");
   assert(RBCCR.covers(*TRI.getRegClass(AArch64::CCRRegClassID)) &&
          "Class not added?");
   assert(RBCCR.getSize() == 32 && "CCR should hold up to 32-bit");
 
   // Check that the TableGen'ed like file is in sync we our expectations.
   // First, the Idx.
   assert(AArch64::PartialMappingIdx::PMI_GPR32 ==
              AArch64::PartialMappingIdx::PMI_FirstGPR &&

lib/Target/ARM/ARMRegisterBankInfo.cpp

const uint32_t GPRCoverageData[] = {
     // Classes 129-160
     0,
     // Classes 161-192
     0,
     // Classes 193-224
     0,
 };
 
-RegisterBank GPRRegBank;
+RegisterBank GPRRegBank(ARM::GPRRegBankID, "GPRB", 32, ARM::GPRCoverageData);
 RegisterBank *RegBanks[] = {&GPRRegBank};
 
 RegisterBankInfo::PartialMapping GPRPartialMapping{0, 32, GPRRegBank};
 
 RegisterBankInfo::ValueMapping ValueMappings[] = {
     {&GPRPartialMapping, 1}, {&GPRPartialMapping, 1}, {&GPRPartialMapping, 1}};
 } // end namespace arm
 } // end namespace llvm
 
 ARMRegisterBankInfo::ARMRegisterBankInfo(const TargetRegisterInfo &TRI)
     : RegisterBankInfo(ARM::RegBanks, ARM::NumRegisterBanks) {
   static bool AlreadyInit = false;
   // We have only one set of register banks, whatever the subtarget
   // is. Therefore, the initialization of the RegBanks table should be
   // done only once. Indeed the table of all register banks
   // (ARM::RegBanks) is unique in the compiler. At some point, it
   // will get tablegen'ed and the whole constructor becomes empty.
   if (AlreadyInit)
     return;
   AlreadyInit = true;
 
-  // Initialize the GPR bank.
-  setRegBankData(ARM::GPRRegBankID, "GPRB", 32, ARM::GPRCoverageData);
   const RegisterBank &RBGPR = getRegBank(ARM::GPRRegBankID);
   (void)RBGPR;
   assert(&ARM::GPRRegBank == &RBGPR && "The order in RegBanks is messed up");
+
+  // Initialize the GPR bank.
   assert(RBGPR.covers(*TRI.getRegClass(ARM::GPRRegClassID)) &&
          "Subclass not added?");
   assert(RBGPR.covers(*TRI.getRegClass(ARM::GPRwithAPSRRegClassID)) &&
          "Subclass not added?");
   assert(RBGPR.covers(*TRI.getRegClass(ARM::GPRnopcRegClassID)) &&
          "Subclass not added?");
   assert(RBGPR.covers(*TRI.getRegClass(ARM::rGPRRegClassID)) &&
          "Subclass not added?");