[PATCH] D27976: [aarch64][globalisel] Move data into <Target>GenRegisterBankInfo. NFC.

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: lib/Target/AArch64/AArch64GenRegisterBankInfo.def:99
+RegisterBank AArch64GenRegisterBankInfo::FPRRegBank(AArch64GenRegisterBankInfo::RegisterBankData[AArch64::FPRRegBankID]);
+RegisterBank AArch64GenRegisterBankInfo::CCRRegBank(AArch64GenRegisterBankInfo::RegisterBankData[AArch64::CCRRegBankID]);
 
----------------
qcolombet wrote:
> I would rather have the register banks in the AArch64 namespace. I expect we will use them directly, and having to specify AArch64GenRegisterBankInfo seems painful.
Hmm, I can move it to llvm::AArch64 but I have to either hard-code the number of register classes and eliminate finishInit() or allow these variables to be in a partially-initialized state until the TargetRegisterInfo exists and something (currently <Target>RegisterBankInfo) can finish the initialization. If we do this after the tablegen patch then we can have tablegen emit the correct size.

Which would you prefer?

Also, just so we're on the same page: Which passes do you expect to use them directly?


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.h:32
 
+class AArch64GenRegisterBankInfo : public RegisterBankInfo {
+private:
----------------
qcolombet wrote:
> FWIW, I believe that generating that part could be part of the tablegen switch. I indeed don't think it makes much to have a XXXGen manually written. I.e., could be squashed with the tablegen switch.
> 
> But I am fine with the intermediate step if it does not last long.
It could be part of the tablegen switch. The reason it isn't is that Ahmed suggested adding the class first and then having a tablegen patch make tablegen emit that same implementation.


================
Comment at: lib/Target/AArch64/AArch64RegisterBankInfo.h:78
+                                      PartialMappingIdx LastAlias,
+                                      std::vector<PartialMappingIdx> Order) {
+    assert(Order.front() == FirstAlias && "First enumerator is not first");
----------------
qcolombet wrote:
> qcolombet wrote:
> > I would prefer that we call this checkXXX and return a bool.
> > The assert is then part of the caller, i.e., assert(checkXXX).
> const std::vector<...> &
> I would prefer that we call this checkXXX and return a bool.
> The assert is then part of the caller, i.e., assert(checkXXX).

Ok.

> const std::vector<...> &

Well spotted


https://reviews.llvm.org/D27976