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

[Quentin Colombet via Phabricator via llvm-commits]

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

Hi Daniel,

LGTM modulo a couple of nitpicks.

Cheers,
-Quentin



================
Comment at: lib/Target/AArch64/AArch64GenRegisterBankInfo.def:99
+RegisterBank AArch64GenRegisterBankInfo::FPRRegBank(AArch64GenRegisterBankInfo::RegisterBankData[AArch64::FPRRegBankID]);
+RegisterBank AArch64GenRegisterBankInfo::CCRRegBank(AArch64GenRegisterBankInfo::RegisterBankData[AArch64::CCRRegBankID]);
 
----------------
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.


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


================
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:
> 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<...> &


https://reviews.llvm.org/D27976