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

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet accepted this revision.
qcolombet added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.h:20
+#define GET_REGBANK_DECLARATIONS
+#include "AArch64GenRegisterBank.inc"
+
----------------
dsanders wrote:
> qcolombet wrote:
> > 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?
> > We probably want GET_TARGET_REGBANK_CLASS as well, aren't we?
> 
> Yes, but it needs to be inside the class declaration for the moment (see below).
> 
> > BTW, do we want to make this distinction?
> 
> When everything is tablegen-erated: Probably not. I don't think there will be a need to keep them separate.
> 
> For the moment, PartialMappingIdx is the main blocking factor that prevents us from tablegen-erating the whole class and merging these two include points. There are enumerators missing (e.g. FPR192) at the moment.
Ok.


================
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:
----------------
dsanders wrote:
> qcolombet wrote:
> > This shouldn't be "AArch64GenRegisterBankInfo : public RegisterBankInfo"
> > But "AArch64RegisterBankInfo : public AArch64GenRegisterBankInfo"
> No, this is the declaration of AArch64GenRegisterBankInfo which will eventually be fully tablegen-erated. AArch64RegisterBankInfo is declared next (line 95 at the moment) as `class AArch64RegisterBankInfo final : public AArch64GenRegisterBankInfo`.
Ah right, I missed the second declaration.


================
Comment at: lib/Target/AArch64/AArch64RegisterBanks.td:11
+//
+//===----------------------------------------------------------------------===//
+
----------------
dsanders wrote:
> qcolombet wrote:
> > I would expect an include of AArch64RegisterInfo.td, otherwise GPR64all and such won't be defined.
> This file is included by AArch64.td which includes AArch64RegisterInfo.td on the line before.
> 
> FWIW, I'm not very keen on all the tablegen files starting from AArch64.td as they currently do. It's wasteful to parse lots of classes and defs that never contribute to the output. We'll need to fix that in tablegen() in our CMake rules before we can start unpicking the dependencies here.
Good point. Agreed.


================
Comment at: utils/TableGen/RegisterBankEmitter.cpp:262
+            Bank.addRegisterClass(RC);
+          });
+    }
----------------
dsanders wrote:
> qcolombet wrote:
> > 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.
> The main reason I wrote it this way was to keep the high-level intent (add all the relevant classes) separate from the complexity of how we determine that a given register class is a member. I also needed to factor the current contents of visitFn() into a function (or triplicate it) and I thought it made most sense to put it here rather than in a static function off to the side.
> 
> I'm happy to fold it into visitRegisterBankClasses() and rename that if that's what you prefer.
Up to you.
I found it easier to have the name of the function directly in the visit method instead of having to look at the call site, but your argument works as well.


https://reviews.llvm.org/D27338