[PATCH] D27338: [globalisel] Tablegen-erate current Register Bank Information

Tue Jan 3 17:21:44 PST 2017

qcolombet added a comment.

Hi Daniel,

Nitpick stuff, the base is indeed sound and I expect we converge on the final patch quickly.

Thanks,
-Quentin

================
Comment at: lib/Target/AArch64/AArch64GenRegisterBankInfo.def:19
+#define GET_TARGET_REGBANK_IMPL
+#include "AArch64GenRegisterBank.inc"

----------------
I would move that into AArch64RegisterBankInfo.cpp, right before the include of the .def.
Eventually, the .def will be removed so let us move all the useful code in the cpp.

================
Comment at: lib/Target/AArch64/AArch64GenRegisterBankInfo.def:38
+    {0, 64, AArch64GenRegisterBankInfo::GPRRegBank},
+};

----------------
Update the "// Idx: " in the comments to match the actual index.

================
Comment at: lib/Target/AArch64/AArch64GenRegisterBankInfo.def:45
     // those mapping).
-    // 0: GPR 32-bit value. <-- This must match First3OpsIdx.
-    {&AArch64GenRegisterBankInfo::PartMappings[PMI_GPR32 - PMI_Min], 1},
-    {&AArch64GenRegisterBankInfo::PartMappings[PMI_GPR32 - PMI_Min], 1},
-    {&AArch64GenRegisterBankInfo::PartMappings[PMI_GPR32 - PMI_Min], 1},
-    // 3: GPR 64-bit value.
-    {&AArch64GenRegisterBankInfo::PartMappings[PMI_GPR64 - PMI_Min], 1},
-    {&AArch64GenRegisterBankInfo::PartMappings[PMI_GPR64 - PMI_Min], 1},
-    {&AArch64GenRegisterBankInfo::PartMappings[PMI_GPR64 - PMI_Min], 1},
     // 6: FPR 32-bit value.
     {&AArch64GenRegisterBankInfo::PartMappings[PMI_FPR32 - PMI_Min], 1},
----------------
Ditto.

================
Comment at: lib/Target/AArch64/AArch64GenRegisterBankInfo.def:89
+    {nullptr, 1},
+    {nullptr, 1},
+    // 21: GPR 32-bit value to FPR 32-bit value. <-- This must match
----------------
Why do we need three of them instead of none previously?

At the very least I would have expected that we need just one of this input.

================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.h:20
+#define GET_REGBANK_DECLARATIONS
+#include "AArch64GenRegisterBank.inc"
+
----------------
We probably want  GET_TARGET_REGBANK_CLASS as well, aren't we?
BTW, do we want to make this distinction?
Or put differently, shouldn't we guard all of those under GET_REGBANKINFO_HEADER so that we have an option to get everything needed without having to put all the macros down?

================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.h:26

-namespace AArch64 {
-enum {
-  GPRRegBankID = 0, /// General Purpose Registers: W, X.
-  FPRRegBankID = 1, /// Floating Point/Vector Registers: B, H, S, D, Q.
-  CCRRegBankID = 2, /// Conditional register: NZCV.
-  NumRegisterBanks
-};
-} // End AArch64 namespace.
-
 class AArch64GenRegisterBankInfo : public RegisterBankInfo {
 private:
----------------
This shouldn't be "AArch64GenRegisterBankInfo : public RegisterBankInfo"
But "AArch64RegisterBankInfo : public AArch64GenRegisterBankInfo"

================
Comment at: lib/Target/AArch64/AArch64RegisterBanks.td:11
+//
+//===----------------------------------------------------------------------===//
+
----------------
I would expect an include of AArch64RegisterInfo.td, otherwise GPR64all and such won't be defined.

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:32
+  /// class is included in the register bank.
+  typedef std::vector<const CodeGenRegisterClass *> RegisterClassesTy;
+
----------------
Which StringRef are you referring to here?

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:44
+  /// mapping. It should be replaced when ExcludePartialMappings is removed.
+  const CodeGenRegisterClass *RCWithLargestRegsSize;
+
----------------
I can see that for debug purposes it is good to get the name of the class with the largest size. Could you update the comment to get rid of the reference to the partial mapping and ExcludePartialMappings stuff, though? 

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:155
+///
+/// A class belongs to the bank iff:
+/// * It is explicitly specified
----------------
I would add "iff any of this applies:"

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:159
+/// * It is a class containing subregisters of the registers of a class that
+///   is a member.
+///
----------------
You can add (a.k.a. subreg-class)

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:162
+/// This function must be called for each explicitly specified register class.
+static void visitRegisterBankClasses(
+    CodeGenRegBank &RegisterClassHierarchy, const CodeGenRegisterClass *RC,
----------------
Add a comment explaining what is RC, Kind, and visitFn.

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:172
+
+    // Visit each subclass of an explicitly named class
+    if (RC != &PossibleSubclass && RC->hasSubClass(&PossibleSubclass))
----------------
Period.

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:178
+
+    // Visit each class that has a subregister of RC
+    for (const auto &SubIdx : RegisterClassHierarchy.getSubRegIndices()) {
----------------
Period.

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:178
+
+    // Visit each class that has a subregister of RC
+    for (const auto &SubIdx : RegisterClassHierarchy.getSubRegIndices()) {
----------------
qcolombet wrote:
> Period.
To be a bit more explicit here (or in the comment of the function) we could call out what it means to be a subreg-class:
PossibleSubclass is a subreg-class iff it exists a sub register index SubIdx such that for each register Reg in RC, Reg:SubIdx is in PossibleSubclass.

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:220
+  OS << "} // end anonymous namespace\n"
+     << "const RegisterBank::DataTy " << TargetName
+     << "GenRegisterBankInfo::RegisterBankData[] = {\n";
----------------
Before emitting the structure, could we emit a line with a comment with the general structure of each line?
E.g.,
<< "// ID, Name, MaxSize, CoveredRegClasses"

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:221
+     << "const RegisterBank::DataTy " << TargetName
+     << "GenRegisterBankInfo::RegisterBankData[] = {\n";
+  for (const auto &Bank : Banks) {
----------------
Following the other review, this should change to directly populate the register banks.

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:226
+    OS << "    {" << QualifiedBankID << ", \"" << Bank.getName() << "\", "
+       << Size << ", " << Bank.getCoverageArrayName() << "},\n";
+  }
----------------
Could you add an assert that the current index in the array is equal to the expected value of the enumerator of the Bank?

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:232
+    OS << "RegisterBank " << TargetName
+       << "GenRegisterBankInfo::" << Bank.getInstanceVarName() << "("
+       << TargetName << "GenRegisterBankInfo::RegisterBankData[" << TargetName
----------------
Following the other review, I would rather have a simple TargetName for the namespace of the register banks.

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:235
+       << "::" << Bank.getEnumeratorName() << "]);\n";
+  OS << "\n";
+
----------------
With the direct initialization of the RegisterBank, we shouldn't need that section anymore, right?

================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:262
+            Bank.addRegisterClass(RC);
+          });
+    }
----------------
Like I mentioned in my previous review, visitRegisterBankClasses is only used with this visitFn, thus, it seems to me like we could drop this parameter and call the proper function directly in visitRegisterBankClasses.

While here, we could rename it addSubClassesAndSubregClasses or something more explicitly like this.

https://reviews.llvm.org/D27338